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

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

On Tue, Nov 21, 2023 at 10:03:01AM -0500, Stewart Hildebrand wrote:
> On 11/21/23 09:45, Roger Pau Monné wrote:
> > On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> >> @@ -407,26 +439,25 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int 
> >> reg, unsigned int size)
> >>  
> >>  /*
> >>   * Perform a maybe partial write to a register.
> >> - *
> >> - * Note that this will only work for simple registers, if Xen needs to
> >> - * trap accesses to rw1c registers (like the status PCI header register)
> >> - * the logic in vpci_write will have to be expanded in order to correctly
> >> - * deal with them.
> >>   */
> >>  static void vpci_write_helper(const struct pci_dev *pdev,
> >>                                const struct vpci_register *r, unsigned int 
> >> size,
> >>                                unsigned int offset, uint32_t data)
> >>  {
> >> +    uint32_t val = 0;
> >> +
> >>      ASSERT(size <= r->size);
> >>  
> >> -    if ( size != r->size )
> >> +    if ( (size != r->size) || r->ro_mask )
> >>      {
> >> -        uint32_t val;
> >> -
> >>          val = r->read(pdev, r->offset, r->private);
> >> +        val &= ~r->rw1c_mask;
> >>          data = merge_result(val, data, size, offset);
> >>      }
> >>  
> >> +    data &= ~(r->rsvdz_mask | r->ro_mask);
> >> +    data |= val & r->ro_mask;
> > 
> > I've been thinking about this, and the way the ro_mask is implemented
> > (and the way we want to handle ro bits) is the same behavior as RsvdP.
> > I would suggest to rename the ro_mask to rsvdp_mask and note
> > that for resilience reasons we will handle RO bits as RsvdP.
> 
> But the reads behave differently. RO should return the value, RsvdP should 
> return 0 when read (according to the PCIe Base Spec 4.0).

Hm, right, sorry for the wrong suggestion.  We should force bits to 0
for guests reads, but make sure that for writes the value on the
hardware is preserved.

So we need the separate mask for RsvdP, which will be handled like
ro_mask in vpci_write_helper() and like rsvdz_mask in vpci_read().

Roger.