Re: [PATCH v7 1/2] xen/vpci: header: status register handler

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 22, 2023 at 03:16:29PM -0500, Stewart Hildebrand wrote:
> On 11/17/23 07:40, Roger Pau Monné wrote:
> > On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> >>      r->write(pdev, r->offset, data & (0xffffffffU >> (32 - 8 * r->size)),
> >>               r->private);
> >>  }
> >> diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
> >> index 84b18736a85d..b72131729db6 100644
> >> --- a/xen/include/xen/pci_regs.h
> >> +++ b/xen/include/xen/pci_regs.h
> >> @@ -66,6 +66,15 @@
> >>  #define  PCI_STATUS_REC_MASTER_ABORT      0x2000 /* Set on master abort */
> >>  #define  PCI_STATUS_SIG_SYSTEM_ERROR      0x4000 /* Set when we drive 
> >> SERR */
> >>  #define  PCI_STATUS_DETECTED_PARITY       0x8000 /* Set on parity error */
> >> +#define  PCI_STATUS_RSVDZ_MASK            0x0006
> > 
> > In my copy of the PCIe 6 spec bit 6 is also RsvdZ, so the mask should
> > be 0x46.
> 
> Right, mine too. It's probably safer to follow the newer version of the spec, 
> so I'll make the change. For completeness / archaeology purposes, I just want 
> to document some relevant data points here.
> 
> In PCIe 4 spec, it says this about bit 6:
> "These bits were used in previous versions of the programming model. Careful 
> consideration should be given to any attempt to repurpose them."
> 
> Going further back, PCI (old school PCI, not Express) spec 3.0 says this 
> about bit 6:
> "This bit is reserved. *"
> "* In Revision 2.1 of this specification, this bit was used to indicate 
> whether or not a device supported User Definable Features."
> 
> Just above in our pci_regs.h (and equally in Linux 
> include/uapi/linux/pci_regs.h) we have this definition for bit 6:
> #define  PCI_STATUS_UDF         0x40    /* Support User Definable Features 
> [obsolete] */
> 
> Qemu hw/xen/xen_pt_config_init.c treats bit 6 as RO:
>         .ro_mask    = 0x06F8,

Right, given the implementation of ro_mask that would likely be fine.
Reading unconditionally as 0 while preserving the value on writes
seems the safest option.

Thanks, Roger.