Re: [Qemu-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Simon Gaiser]

Jason Andryuk:
> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <address@hidden> wrote:
>>
>>> -----Original Message-----
>>> From: Jason Andryuk [mailto:address@hidden
>>> Sent: 11 March 2019 18:02
>>> To: address@hidden
>>> Cc: address@hidden; address@hidden; Simon Gaiser
>>> <address@hidden>; Jason Andryuk <address@hidden>; Stefano Stabellini
>>> <address@hidden>; Anthony Perard <address@hidden>; Paul Durrant
>>> <address@hidden>
>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>
>>> From: Simon Gaiser <address@hidden>
>>>
>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
>>> an address which is not page aligned.
>>
>> IIRC the PCI spec says that the minimum memory region size should be at 
>> least 4k. Should we even be tolerating BARs smaller than that?
>>
>>   Paul
>>
> 
> Hi, Paul.
> 
> Simon found this, so it affects a real device.  Simon, do you recall
> which device was affected?

Not sure which one it was. Probably the USB controller or the SD host
controller. As your example below shows this is not so uncommon.

> I think BARs only need to be power-of-two size and aligned, and 4k is
> not a minimum.  16bytes may be a minimum, but I don't know what the
> spec says.
> 
> On an Ivy Bridge system, here are some of the devices with BARs smaller than 
> 4K:
> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> Series Chipset Family MEI Controller #1 (rev 04)
>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> SMBus Controller (rev 04)
>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> Controller (rev 30)
>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> 
> These examples are all 4K aligned, so this is not an issue on this machine.
> 
> Reviewing the code, I'm now wondering if the following in
> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> xc_domain_memory_mapping(xen_xc, xen_domid,
>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
>                                      op);
> 
> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> in would be 0xd0501000 which is past the actual location.  Should the
> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> 
> BARs smaller than a page would also be a problem if BARs for different
> devices shared the same page.
> 
> Regards,
> Jason
> 
>>> This breaks the memory mapping via
>>> xc_domain_memory_mapping since this function is page based and the
>>> "offset" is therefore lost.
>>>
>>> Without this patch you will see error like this in the stubdom log:
>>>
>>>   [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. 
>>> @0x0000000000000004
>>>
>>> QubesOS/qubes-issues#2849
>>>
>>> Signed-off-by: Simon Gaiser <address@hidden>
>>> Signed-off-by: Jason Andryuk <address@hidden>
>>> ---
>>>  hw/xen/xen_pt.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>>> index 5539d56c3a..7f680442ee 100644
>>> --- a/hw/xen/xen_pt.c
>>> +++ b/hw/xen/xen_pt.c
>>> @@ -449,9 +449,10 @@ static int 
>>> xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>>>      /* Register PIO/MMIO BARs */
>>>      for (i = 0; i < PCI_ROM_SLOT; i++) {
>>>          XenHostPCIIORegion *r = &d->io_regions[i];
>>> +        pcibus_t r_size = r->size;
>>>          uint8_t type;
>>>
>>> -        if (r->base_addr == 0 || r->size == 0) {
>>> +        if (r->base_addr == 0 || r_size == 0) {
>>>              continue;
>>>          }
>>>
>>> @@ -469,15 +470,18 @@ static int 
>>> xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>>>                  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>>>              }
>>>              *cmd |= PCI_COMMAND_MEMORY;
>>> +
>>> +            /* Round up to a full page for the hypercall. */
>>> +            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
>>>          }
>>>
>>>          memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
>>> -                              "xen-pci-pt-bar", r->size);
>>> +                              "xen-pci-pt-bar", r_size);
>>>          pci_register_bar(&s->dev, i, type, &s->bar[i]);
>>>
>>>          XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
>>>                     " base_addr=0x%08"PRIx64" type: %#x)\n",
>>> -                   i, r->size, r->base_addr, type);
>>> +                   i, r_size, r->base_addr, type);
>>>      }
>>>
>>>      /* Register expansion ROM address */
>>> --
>>> 2.20.1

signature.asc
Description: OpenPGP digital signature