[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH] transitional issues: add new IDs for all devices
On Wed, Sep 25, 2013 at 01:50:04PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Tue, Sep 24, 2013 at 09:10:09PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > non-transitional devices should have been able to simply update
> >> > revision ID to make sure legacy drivers are not loaded.
> >> > Unfortunately, mistakes were made:
> >> > - we didn't stress that drivers must check revision ID,
> >> > and of course there's no easy way for drivers to
> >> > test this failure path,
> >> > so older versions of Windows drivers ignored revision
> >> > (latest vision matches revision correctly)
> >> > - CCW lacks revision ID field
> >> >
> >> > Both facts mean a non-transitional device would need
> >> > a separate mechanism to prevent legacy drivers from
> >> > loading.
> >> > We aren't running out of device IDs yet, so
> >> > let's use up some to resolve this.
> >> >
> >> > I incremented all IDs by 0x100 intentionally -
> >> > for the PCI bindings, this should help remind people they can't
> >> > just stick the Subsystem ID into the low byte of the Device ID.
> >>
> >> I don't think this actually works.
> >
> > Hmm what doesn't work, exactly?
> >
> >> If you offer a non-transitional device to a legacy driver, it will
> >> fail anyway (device doesn't work due to missing feature 32).
> >
> > How does it fail? Legacy drivers don't give devices any chance
> > to fail gracefully.
>
> >
> >> There's not much difference between that and it not finding the device.
> >
> > Imagine a non transitional device.
> > It would want to use offset 0 in BAR0 for something else not config.
> > When legacy driver attempts to access it, it can cause
> > all kind of mischief.
>
> As per call, I think we should check that the existing Windows drivers
> fail if BAR0 does not exist, and then make that a recommendation for
> PCI, eg:
>
> On a platform where legacy devices can exist, devices should not offer
> BAR0 at all.
>
> Cheers,
> Rusty.
Heh no luck at all. My assumption is we'll want to have IO BARs
somewhere. If that happens drivers do all kind of weird stuff.
For example:
***********************************************************/
static BOOLEAN GetAdapterResources(PNDIS_RESOURCE_LIST RList, tAdapterResources *pResources)
{
UINT i;
NdisZeroMemory(pResources, sizeof(*pResources));
for (i = 0; i < RList->Count; ++i)
{
ULONG type = RList->PartialDescriptors[i].Type;
if (type == CmResourceTypePort)
{
PHYSICAL_ADDRESS Start = RList->PartialDescriptors[i].u.Port.Start;
ULONG len = RList->PartialDescriptors[i].u.Port.Length;
DPrintf(0, ("Found IO ports at %08lX(%d)", Start.LowPart, len));
pResources->ulIOAddress = Start.LowPart;
pResources->IOLength = len;
}
else if (type == CmResourceTypeInterrupt)
{
pResources->Vector = RList->PartialDescriptors[i].u.Interrupt.Vector;
pResources->Level = RList->PartialDescriptors[i].u.Interrupt.Level;
pResources->Affinity = RList->PartialDescriptors[i].u.Interrupt.Affinity;
pResources->InterruptFlags = RList->PartialDescriptors[i].Flags;
DPrintf(0, ("Found Interrupt vector %d, level %d, affinity %X, flags %X",
pResources->Vector, pResources->Level, (ULONG)pResources->Affinity, pResources->InterruptFlags));
}
}
return pResources->ulIOAddress && pResources->Vector && pResources->IOLength == 32;
}
So it tries really hard to find IO resources somewhere,
once found uses the first IO BAR with the requirement that it's
size is 32.
Yes we could ask devices to have an IO BAR with size 1 but that's
very ugly.
Balloon is more flexible: it looks for first IO BAR with size >= 32:
for (i=0; i < WdfCmResourceListGetCount(ResourceListTranslated); i++) {
desc = WdfCmResourceListGetDescriptor( ResourceListTranslated, i );
if(!desc) {
TraceEvents(TRACE_LEVEL_ERROR, DBG_PNP,
"WdfResourceCmGetDescriptor failed\n");
return STATUS_DEVICE_CONFIGURATION_ERROR;
}
switch (desc->Type) {
case CmResourceTypePort:
if (!foundPort &&
desc->u.Port.Length >= 0x20) {
devCtx->PortMapped =
(desc->Flags & CM_RESOURCE_PORT_IO) ? FALSE : TRUE;
PortBasePA = desc->u.Port.Start;
PortLength = desc->u.Port.Length;
foundPort = TRUE;
if (devCtx->PortMapped) {
devCtx->PortBase =
(PUCHAR) MmMapIoSpace( PortBasePA, PortLength, MmNonCached );
if (!devCtx->PortBase) {
TraceEvents(TRACE_LEVEL_ERROR, DBG_PNP, " Unable to map port range %08I64X, length %d\n",
PortBasePA.QuadPart, PortLength);
return STATUS_INSUFFICIENT_RESOURCES;
}
devCtx->PortCount = PortLength;
} else {
devCtx->PortBase = (PUCHAR)(ULONG_PTR) PortBasePA.QuadPart;
devCtx->PortCount = PortLength;
}
}
TraceEvents(TRACE_LEVEL_INFORMATION, DBG_PNP, "<-> Port Resource [%08I64X-%08I64X]\n",
desc->u.Port.Start.QuadPart,
desc->u.Port.Start.QuadPart +
desc->u.Port.Length);
break;
So we'll have to burn up 32 bytes per device otherwise it ignores the BAR.
Block driver is even less restrictive, it does not check size
at all:
for (i = 0; i < nListSize; i++)
{
if(pResDescriptor = WdfCmResourceListGetDescriptor(ResourcesTranslated, i))
{
switch(pResDescriptor->Type)
{
case CmResourceTypePort :
pContext->bPortMapped = (pResDescriptor->Flags & CM_RESOURCE_PORT_IO) ? FALSE : TRUE;
pContext->PortBasePA = pResDescriptor->u.Port.Start;
pContext->uPortLength = pResDescriptor->u.Port.Length;
TraceEvents(TRACE_LEVEL_INFORMATION, DBG_PNP, "IO Port Info [%08I64X-%08I64X]\n",
pResDescriptor->u.Port.Start.QuadPart,
pResDescriptor->u.Port.Start.QuadPart +
pResDescriptor->u.Port.Length);
if (pContext->bPortMapped )
{
pContext->pPortBase = MmMapIoSpace(pContext->PortBasePA,
pContext->uPortLength,
MmNonCached);
if (!pContext->pPortBase) {
TraceEvents(TRACE_LEVEL_ERROR, DBG_HW_ACCESS, "%s>>> Failed to map IO port!\n", __FUNCTION__);
return STATUS_INSUFFICIENT_RESOURCES;
}
}
else
{
pContext->pPortBase = (PVOID)(ULONG_PTR)pContext->PortBasePA.QuadPart;
}
bPortFound = TRUE;
break;
case CmResourceTypeInterrupt:
break;
}
}
}
This one looks like it will happily use the first valid resource it
finds, without checking size. We'd need to make BAR0
large to make it not crash on access, size 1 won't do.
The way I see it, spec explicitly said BAR0 is there,
it didn't say drivers should check that it's there :)
--
MST
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]