[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
  • Date: Mon, 11 Sep 2023 14:16:13 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=X7QQsQC8PI4iYV5qcLG2boDW5rT4GlLbXvyRbJx528U=; b=Grnk6oCzJpD1a3MrQy+YvOOOk6JKFUxbjydAsRUd+FakAq5nD9RG+AgbCSxU080INnKqGXxyO44KVFC43f5UvgdfhlI31AHK0vxqxXzqEBYTso9WMq7pmX/KjgWDyFBFQKN8kABdGNBtC/Rj6j473o4QeJo9sprsu/yMiZmsLp/2RfyIibwB0QA6+fSaf6SQ3TFffco4M/VGvoQpf/mifS1GWtjPErrEb6q4Tro67aiWLv+l0R5tLD7Zv7RkN8FRRQ1+9EZsWNR06m55vhv6ZvZLQrrHr5KOZqd1Ex0FLvoaiL50uyK+MmQ8x1eGz8/l++SxnfLkARTSrvXY2fNHOg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q5TVnxbq20FeYxQVLPVvgZO9UnRkr9/EQcVOh99n626p8QOdhF0XYEjfGgHVi4S4jwbSGOR0gBuD/d/8UfwQherq5KVncY7W2Oz6PaBhE17YcyU5+UbXC+bATomQKx7DQIxQnzdm31eY+86t6GnvCkClpKPm/wIee6G2nlcT4Xpe0rSLKwsY61FNBJGe6A12oGAMUbtYu0PBGD4VQZ+6giSQFbTma3EjfU5Vmphc3aLoLLeb6qZj3PrWPpVO6BW9vyE/Ghqfc7apIY2bRG5HcvvUt3itqnpGylMWcdalHU0tN/fdwrVJbVLCWz8iRslK5VwwBkXt9YCWocZ+01waMQ==
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 11 Sep 2023 18:16:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.