On 8/12/2025 7:41 PM, Wei Liu wrote:
> On Tue, Aug 12, 2025 at 05:22:29PM -0700, Nuno Das Neves wrote:
>> On 7/17/2025 11:55 PM, mhkelle...@gmail.com wrote:
>>> From: Michael Kelley <mhkli...@outlook.com>
>>>
>>> Update hypercall call sites to use the new hv_setup_*() functions
>>> to set up hypercall arguments. Since these functions zero the
>>> fixed portion of input memory, remove now redundant calls to memset()
>>> and explicit zero'ing of input fields.
>>>
>>> Signed-off-by: Michael Kelley <mhkli...@outlook.com>
>>> Reviewed-by: Nuno Das Neves <nunodasne...@linux.microsoft.com>
>>> ---
>>>
>>> Notes:
>>> Changes in v4:
>>> * Rename hv_hvcall_*() functions to hv_setup_*() [Easwar Hariharan]
>>> * Rename hv_hvcall_in_batch_size() to hv_get_input_batch_size()
>>> [Easwar Hariharan]
>>>
>>> Changes in v2:
>>> * Fixed get_vtl() and hv_vtl_apicid_to_vp_id() to properly treat the
>>> input
>>> and output arguments as arrays [Nuno Das Neves]
>>> * Enhanced __send_ipi_mask_ex() and hv_map_interrupt() to check the
>>> number
>>> of computed banks in the hv_vpset against the batch_size. Since an
>>> hv_vpset currently represents a maximum of 4096 CPUs, the hv_vpset
>>> size
>>> does not exceed 512 bytes and there should always be sufficent space.
>>> But
>>> do the check just in case something changes. [Nuno Das Neves]
>>>
>>
>> <snip>
>>
>>> diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
>>> index 090f5ac9f492..87ebe43f58cf 100644
>>> --- a/arch/x86/hyperv/irqdomain.c
>>> +++ b/arch/x86/hyperv/irqdomain.c
>>> @@ -21,15 +21,15 @@ static int hv_map_interrupt(union hv_device_id
>>> device_id, bool level,
>>> struct hv_device_interrupt_descriptor *intr_desc;
>>> unsigned long flags;
>>> u64 status;
>>> - int nr_bank, var_size;
>>> + int batch_size, nr_bank, var_size;
>>>
>>> local_irq_save(flags);
>>>
>>> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>>> - output = *this_cpu_ptr(hyperv_pcpu_output_arg);
>>> + batch_size = hv_setup_inout_array(&input, sizeof(*input),
>>> +
>>> sizeof(input->interrupt_descriptor.target.vp_set.bank_contents[0]),
>>> + &output, sizeof(*output), 0);
>>>
>>
>> Hi Michael, I finally managed to test this series on (nested) root
>> partition and encountered an issue when I applied this patch.
>>
>> With the above change, I saw HV_STATUS_INVALID_ALIGNMENT from this
>> hypercall. I printed out the addresses and sizes and everything looked
>> correct. The output seemed to be correctly placed at the end of the
>> percpu page. E.g. if input was allocated at an address ending in 0x3000,
>> output would be at 0x3ff0, because hv_output_map_device_interrupt is
>> 0x10 bytes in size.
>>
>> But it turns out, the definition for hv_output_map_device_interrupt
>> is out of date (or was never correct)! It should be:
>>
>> struct hv_output_map_device_interrupt {
>> struct hv_interrupt_entry interrupt_entry;
>> u64 extended_status_deprecated[5];
>> } __packed;
>>
>> (The "extended_status_deprecated" field is missing in the current code.)
>>
>> Due to this, when the hypervisor validates the hypercall input/output,
>> it sees that output is going across a page boundary, because it knows
>> sizeof(hv_output_map_device_interrupt) is actually 0x58.
>>
>> I confirmed that adding the "extended_status_deprecated" field fixes the
>> issue. That should be fixed either as part of this patch or an additional
>> one.
>
> Thanks for testing this, Nuno. In that case, can you please submit a
> patch for hv_output_map_device_interrupt? That can go in via the fixes
> tree.
>
> Thanks,
> Wei
>
Sent the fix:
https://lore.kernel.org/linux-hyperv/1755109257-6893-1-git-send-email-nunodasne...@linux.microsoft.com/T/#u
>>
>> Nuno
>>
>> PS. I have yet to test the mshv driver changes in patch 6, I'll try to
>> do so this week.
>>
>>> intr_desc = &input->interrupt_descriptor;
>>> - memset(input, 0, sizeof(*input));
>>> input->partition_id = hv_current_partition_id;
>>> input->device_id = device_id.as_uint64;
>>> intr_desc->interrupt_type = HV_X64_INTERRUPT_TYPE_FIXED;
>>> @@ -41,7 +41,6 @@ static int hv_map_interrupt(union hv_device_id device_id,
>>> bool level,
>>> else
>>> intr_desc->trigger_mode = HV_INTERRUPT_TRIGGER_MODE_EDGE;
>>>
>>> - intr_desc->target.vp_set.valid_bank_mask = 0;
>>> intr_desc->target.vp_set.format = HV_GENERIC_SET_SPARSE_4K;
>>> nr_bank = cpumask_to_vpset(&(intr_desc->target.vp_set),
>>> cpumask_of(cpu));
>>> if (nr_bank < 0) {
>>> @@ -49,6 +48,11 @@ static int hv_map_interrupt(union hv_device_id
>>> device_id, bool level,
>>> pr_err("%s: unable to generate VP set\n", __func__);
>>> return -EINVAL;
>>> }
>>> + if (nr_bank > batch_size) {
>>> + local_irq_restore(flags);
>>> + pr_err("%s: nr_bank too large\n", __func__);
>>> + return -EINVAL;
>>> + }
>>> intr_desc->target.flags = HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
>>>
>>> /*
>>> @@ -78,9 +82,8 @@ static int hv_unmap_interrupt(u64 id, struct
>>> hv_interrupt_entry *old_entry)
>>> u64 status;
>>>
>>> local_irq_save(flags);
>>> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>>>
>>> - memset(input, 0, sizeof(*input));
>>> + hv_setup_in(&input, sizeof(*input));
>>> intr_entry = &input->interrupt_entry;
>>> input->partition_id = hv_current_partition_id;
>>> input->device_id = id;
>>
>>
