Re: [PATCH v6 3/4] xen/vpci: header: status register handler

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 9/11/23 07:10, Jan Beulich wrote:
> On 09.09.2023 04:16, Stewart Hildebrand wrote:
>> @@ -544,6 +545,18 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>      if ( rc )
>>          return rc;
>>
>> +    /*
>> +     * If mask_cap_list is true, PCI_STATUS_CAP_LIST will be set in both
>> +     * rsvdz_mask and ro_mask, and thus will effectively behave as rsvdp
>> +     * (reserved/RAZ and preserved on write).
>> +     */
>> +    rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
>> +                                PCI_STATUS, 2, header, 
>> PCI_STATUS_RSVDZ_MASK |
>> +                                (mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
>> +                                PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
> 
> While not formally disallowed by ./CODING_STYLE, I think this kind of argument
> wrapping is bad practice. If an argument is too long for the current line, it
> should start on a fresh one. Otherwise at the very least I think we'd want the
> continuation to stand out, by parenthesizing the entire argument, thus leading
> to one deeper indentation on the continuing line(s). (This may even be useful
> to do when starting on a fresh line.)

I will change it to this:
    /*
     * Utilize rsvdz_mask to hide PCI_STATUS_CAP_LIST from the guest for now. If
     * support for rsvdp (reserved & preserved) is added in the future, the
     * rsvdp mask should be used instead.
     */
    rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
                                PCI_STATUS, 2, NULL,
                                PCI_STATUS_RSVDZ_MASK |
                                    (mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
                                PCI_STATUS_RO_MASK &
                                    ~(mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
                                PCI_STATUS_RW1C_MASK);

>> @@ -155,7 +165,8 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
>> *read_handler,
>>      /* Some sanity checks. */
>>      if ( (size != 1 && size != 2 && size != 4) ||
>>           offset >= PCI_CFG_SPACE_EXP_SIZE || (offset & (size - 1)) ||
>> -         (!read_handler && !write_handler) )
>> +         (!read_handler && !write_handler) || (ro_mask & rw1c_mask) ||
>> +         (rsvdz_mask & rw1c_mask) )
>>          return -EINVAL;
> 
> What about rsvdz_mask and ro_mask? They better wouldn't overlap either
> (requiring an adjustment to the code you add to init_bars()).

I will add a case for (rsvdz_mask & ro_mask). See above for adjustment in 
init_bars().

>> @@ -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;
> 
> ~(r->rsvdz_mask | r->ro_mask) ? But maybe that's indeed just me ...

I will make this change.