On 5/7/25 03:44, Roger Pau Monné wrote:
> On Tue, May 06, 2025 at 11:05:13PM -0400, Stewart Hildebrand wrote:
>> On 5/6/25 07:16, Roger Pau Monné wrote:
>>> On Fri, Apr 18, 2025 at 02:58:37PM -0400, Stewart Hildebrand wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>> static int vpci_register_cmp(const struct vpci_register *r1,
>>>> const struct vpci_register *r2)
>>>> {
>>>> @@ -438,7 +473,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
>>>> unsigned int size)
>>>> const struct pci_dev *pdev;
>>>> const struct vpci_register *r;
>>>> unsigned int data_offset = 0;
>>>> - uint32_t data = ~(uint32_t)0;
>>>> + uint32_t data = 0xffffffffU >> (32 - 8 * size);
>>>
>>> This seems kind of unrelated to the rest of the code in the patch,
>>> why is this needed? Isn't it always fine to return all ones, and let
>>> the caller truncate to the required size?
>>>
>>> Otherwise the code in vpci_read_hw() also needs to be adjusted.
>>
>> On Arm, since 9a5e22b64266 ("xen/arm: check read handler behavior") we
>> assert that the read handlers don't set any bits above the access size.
>
> I see. That kind of diverges from x86 behavior, that AFAICT (see
> memcpy() at tail of hvmemul_do_io()) instead truncates the memcpy to
> the size of the access.
>
> Maybe it would be better to instead of asserting just truncate the
> returned value to the given size, as that would allow to just return
> ~0 from handlers without having to care about the specific access
> size.
The impression I get from [0] is that that on Arm, there's no benefit to
performing truncation in xen/arch/arm/io.c. Doing so would needlessly
affect other Arm internal read handlers (e.g. vGIC). For vPCI
specifically, however, we could potentially perform truncation in
xen/arch/arm/vpci.c. So I guess it's a question of whether we want to
give special treatment to vPCI compared to all other read handlers on
Arm?
>> I had adjusted data here due to returning it directly from vpci_read()
>> in the current form of the patch. With your suggestion below we would
>> only need to adjust vpci_read_hw() (and then data here would not
>> strictly need adjusting).
>
> Both returns would need adjusting IMO,
OK
> and it should have been part of
> 9a5e22b64266 I think, since that's the commit that introduced the
> checking.
If we proceed with adjusting vpci_read() and vpci_read_hw(): are you OK
with these adjustments included in this patch, or would you prefer them
being split into a pre-patch?
[0]
https://lore.kernel.org/xen-devel/20240522225927.77398-1-stewart.hildebr...@amd.com/T/#t
