Re: [PATCH v1 5/6] x86/hyperv: Implement hypervisor ram collection into vmcore

[Mukesh R <mrathor@linux.microsoft.com>]

On 9/16/25 18:13, Mukesh R wrote:
> On 9/15/25 10:55, Michael Kelley wrote:
>> From: Mukesh Rathor <mrathor@linux.microsoft.com> Sent: Tuesday, September 9, 2025 5:10 PM
>>>
>>> Introduce a new file to implement collection of hypervisor ram into the
>>
>> s/ram/RAM/ (multiple places)
> 
> a quick grep indicates using saying ram is common, i like ram over RAM
> 
>>> vmcore collected by linux. By default, the hypervisor ram is locked, ie,
>>> protected via hw page table. Hyper-V implements a disable hypercall which
>>
>> The terminology here is a bit confusing since you have two names for
>> the same thing: "disable" hypervisor, and "devirtualize". Is it possible to
>> just use "devirtualize" everywhere, and drop the "disable" terminology?
> 
> The concept is devirtualize and the actual hypercall was originally named
> disable. so intermixing is natural imo.

[snip]

>>> +
>>> +/*
>>> + * Setup a temporary gdt to allow the asm code to switch to the long mode.
>>> + * Since the asm code is relocated/copied to a below 4G page, it cannot use rip
>>> + * relative addressing, hence we must use trampoline_pa here. Also, save other
>>> + * info like jmp and C entry targets for same reasons.
>>> + *
>>> + * Returns: 0 on success, -1 on error
>>> + */
>>> +static int hv_crash_setup_trampdata(u64 trampoline_va)
>>> +{
>>> +	int size, offs;
>>> +	void *dest;
>>> +	struct hv_crash_tramp_data *tramp;
>>> +
>>> +	/* These must match exactly the ones in the corresponding asm file */
>>> +	BUILD_BUG_ON(offsetof(struct hv_crash_tramp_data, tramp32_cr3) != 0);
>>> +	BUILD_BUG_ON(offsetof(struct hv_crash_tramp_data, kernel_cr3) != 8);
>>> +	BUILD_BUG_ON(offsetof(struct hv_crash_tramp_data, gdtr32.limit) != 18);
>>> +	BUILD_BUG_ON(offsetof(struct hv_crash_tramp_data,
>>> +						     cs_jmptgt.address) != 40);
>>
>> It would be nice to pick up the constants from a #include file that is
>> shared with the asm code in Patch 4 of the series.
> 
> yeah, could go either way, some don't like tiny headers...  if there are
> no objections to new header for this, i could go that way too.


yeah, i experimented with creating a new header or try to add to existing.
new header doesn't make sense for just 5 #defines, adding C struct there
is not a great idea given it's scope is limited to the specific function
in the c file. adding to another header results in ifdefs for ASM/KERNEL,
so not really worth it. I think for now it is ok, we can live with it.
If arm ends up adding more declarations, we can look into it.


Thanks,
-Mukesh

[ .. deleted.. ]