* Re: [PATCH v5 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Nuno Das Neves @ 2025-07-17 16:21 UTC (permalink / raw)
To: Michael Kelley, Naman Jain, K . Y . Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui
Cc: Roman Kisel, Anirudh Rayabharam, Saurabh Sengar,
Stanislav Kinsburskii, ALOK TIWARI, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org
In-Reply-To: <SN6PR02MB4157F9F1F8493C74C9FCC6E4D449A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 7/9/2025 10:19 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, June 11, 2025 12:27 AM
>> +
>> +union mshv_synic_overlay_page_msr {
>> + u64 as_uint64;
>> + struct {
>> + u64 enabled: 1;
>> + u64 reserved: 11;
>> + u64 pfn: 52;
>> + };
>
> Since this appear to be a Hyper-V synthetic MSR, add __packed?
>
>> +};
>> +
>> +union hv_register_vsm_capabilities {
>> + u64 as_uint64;
>> + struct {
>> + u64 dr6_shared: 1;
>> + u64 mbec_vtl_mask: 16;
>> + u64 deny_lower_vtl_startup: 1;
>> + u64 supervisor_shadow_stack: 1;
>> + u64 hardware_hvpt_available: 1;
>> + u64 software_hvpt_available: 1;
>> + u64 hardware_hvpt_range_bits: 6;
>> + u64 intercept_page_available: 1;
>> + u64 return_action_available: 1;
>> + u64 reserved: 35;
>> + } __packed;
>> +};
>> +
>> +union hv_register_vsm_page_offsets {
>> + struct {
>> + u64 vtl_call_offset : 12;
>> + u64 vtl_return_offset : 12;
>> + u64 reserved_mbz : 40;
>> + };
>> + u64 as_uint64;
>> +} __packed;
>
> We've usually put the __packed on the struct definition. Consistency .... :-)
>
> Don't these three register definitions belong somewhere in the
> hvhdk or hvgdk include files?
>
I agree, hv_register_vsm_capabilities and hv_register_vsm_page_offsets
can be moved to the appropriate include/hyperv/ header/s.
Regarding mshv_synic_overlay_page_msr, it is a generic structure that
appears to be used for several overlay page MSRs (SIMP, SIEF, etc).
But, the type doesn't appear in the hv*dk headers explicitly; it's just
used internally by the hypervisor.
I think it should be renamed with a hv_ prefix to indicate it's part of
the hypervisor ABI, and a brief comment with the provenance:
/* SYNIC_OVERLAY_PAGE_MSR - internal, identical to hv_synic_simp */
union hv_synic_overlay_page_msr {
/* <snip> */
};
And I'm fine with it staying in this file since it's only used here right
now, and doesn't really come from the one of the hyperv headers.
Nuno
^ permalink raw reply
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: Michael Kelley @ 2025-07-17 17:25 UTC (permalink / raw)
To: Dan Williams, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Manivannan Sadhasivam,
Rob Herring, K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <20250716160835.680486-3-dan.j.williams@intel.com>
From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, July 16, 2025 9:09 AM
>
> The ability to emulate a host bridge is useful not only for hardware PCI
> controllers like CONFIG_VMD, or virtual PCI controllers like
> CONFIG_PCI_HYPERV, but also for test and development scenarios like
> CONFIG_SAMPLES_DEVSEC [1].
>
> One stumbling block for defining CONFIG_SAMPLES_DEVSEC, a sample
> implementation of a platform TSM for PCI Device Security, is the need to
> accommodate PCI_DOMAINS_GENERIC architectures alongside x86 [2].
>
> In support of supplementing the existing CONFIG_PCI_BRIDGE_EMUL
> infrastructure for host bridges:
>
> * Introduce pci_bus_find_emul_domain_nr() as a common way to find a free
> PCI domain number whether that is to reuse the existing dynamic
> allocation code in the !ACPI case, or to assign an unused domain above
> the last ACPI segment.
>
> * Convert pci-hyperv to the new allocator so that the PCI core can
> unconditionally assume that bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET
> is the dynamically allocated case.
>
> A follow on patch can also convert vmd to the new scheme. Currently vmd
> is limited to CONFIG_PCI_DOMAINS_GENERIC=n (x86) so, unlike pci-hyperv,
> it does not immediately conflict with this new
> pci_bus_find_emul_domain_nr() mechanism.
>
> Link: https://lore.kernel.org/all/174107249038.1288555.12362100502109498455.stgit@dwillia2-xfh.jf.intel.com/ [1]
> Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Closes: https://lore.kernel.org/all/20250311144601.145736-3-suzuki.poulose@arm.com/
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Dexuan Cui <decui@microsoft.com>
> Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/pci/controller/pci-hyperv.c | 53 ++---------------------------
> drivers/pci/pci.c | 43 ++++++++++++++++++++++-
> drivers/pci/probe.c | 8 ++++-
> include/linux/pci.h | 4 +++
> 4 files changed, 56 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index ef5d655a0052..cfe9806bdbe4 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -3630,48 +3630,6 @@ static int hv_send_resources_released(struct hv_device *hdev)
> return 0;
> }
>
> -#define HVPCI_DOM_MAP_SIZE (64 * 1024)
> -static DECLARE_BITMAP(hvpci_dom_map, HVPCI_DOM_MAP_SIZE);
> -
> -/*
> - * PCI domain number 0 is used by emulated devices on Gen1 VMs, so define 0
> - * as invalid for passthrough PCI devices of this driver.
> - */
> -#define HVPCI_DOM_INVALID 0
> -
> -/**
> - * hv_get_dom_num() - Get a valid PCI domain number
> - * Check if the PCI domain number is in use, and return another number if
> - * it is in use.
> - *
> - * @dom: Requested domain number
> - *
> - * return: domain number on success, HVPCI_DOM_INVALID on failure
> - */
> -static u16 hv_get_dom_num(u16 dom)
> -{
> - unsigned int i;
> -
> - if (test_and_set_bit(dom, hvpci_dom_map) == 0)
> - return dom;
> -
> - for_each_clear_bit(i, hvpci_dom_map, HVPCI_DOM_MAP_SIZE) {
> - if (test_and_set_bit(i, hvpci_dom_map) == 0)
> - return i;
> - }
> -
> - return HVPCI_DOM_INVALID;
> -}
> -
> -/**
> - * hv_put_dom_num() - Mark the PCI domain number as free
> - * @dom: Domain number to be freed
> - */
> -static void hv_put_dom_num(u16 dom)
> -{
> - clear_bit(dom, hvpci_dom_map);
> -}
> -
> /**
> * hv_pci_probe() - New VMBus channel probe, for a root PCI bus
> * @hdev: VMBus's tracking struct for this root PCI bus
> @@ -3715,9 +3673,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> * collisions) in the same VM.
> */
> dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> - dom = hv_get_dom_num(dom_req);
> + dom = pci_bus_find_emul_domain_nr(dom_req);
>
> - if (dom == HVPCI_DOM_INVALID) {
> + if (dom < 0) {
> dev_err(&hdev->device,
> "Unable to use dom# 0x%x or other numbers", dom_req);
> ret = -EINVAL;
> @@ -3851,7 +3809,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> destroy_wq:
> destroy_workqueue(hbus->wq);
> free_dom:
> - hv_put_dom_num(hbus->bridge->domain_nr);
> + pci_bus_release_emul_domain_nr(hbus->bridge->domain_nr);
> free_bus:
> kfree(hbus);
> return ret;
> @@ -3976,8 +3934,6 @@ static void hv_pci_remove(struct hv_device *hdev)
> irq_domain_remove(hbus->irq_domain);
> irq_domain_free_fwnode(hbus->fwnode);
>
> - hv_put_dom_num(hbus->bridge->domain_nr);
> -
> kfree(hbus);
> }
>
> @@ -4148,9 +4104,6 @@ static int __init init_hv_pci_drv(void)
> if (ret)
> return ret;
>
> - /* Set the invalid domain number's bit, so it will not be used */
> - set_bit(HVPCI_DOM_INVALID, hvpci_dom_map);
> -
> /* Initialize PCI block r/w interface */
> hvpci_block_ops.read_block = hv_read_config_block;
> hvpci_block_ops.write_block = hv_write_config_block;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113b..833ebf2d5213 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6692,9 +6692,50 @@ static void pci_no_domains(void)
> #endif
> }
>
> +#ifdef CONFIG_PCI_DOMAINS
> +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> +
> +/*
> + * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> + * fallback to the first free domain number above the last ACPI segment number.
> + * Caller may have a specific domain number in mind, in which case try to
> + * reserve it.
> + *
> + * Note that this allocation is freed by pci_release_host_bridge_dev().
> + */
> +int pci_bus_find_emul_domain_nr(int hint)
> +{
> + if (hint >= 0) {
> + hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> + GFP_KERNEL);
This almost preserves the existing functionality in pci-hyperv.c. But if the
"hint" passed in is zero, current code in pci-hyperv.c treats that as a
collision and allocates some other value. The special treatment of zero is
necessary per the comment with the definition of HVPCI_DOM_INVALID.
I don't have an opinion on whether the code here should treat a "hint"
of zero as invalid, or whether that should be handled in pci-hyperv.c.
> +
> + if (hint >= 0)
> + return hint;
> + }
> +
> + if (acpi_disabled)
> + return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> +
> + /*
> + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> + * which the lower 16 bits are the PCI Segment Group (domain) number.
> + * Other bits are currently reserved.
> + */
Back in 2018 and 2019, the Microsoft Linux team encountered problems with
PCI domain IDs that exceeded 0xFFFF. User space code, such as the Xorg X server,
assumed PCI domain IDs were at most 16 bits, and retained only the low 16 bits
if the value was larger. My memory of the details is vague, but I believe some
or all of this behavior was tied to libpciaccess. As a result of these user space
limitations, the pci-hyperv.c code made sure to not create any domain IDs
larger than 0xFFFF. The problem was not just theoretical -- Microsoft had
customers reporting issues due to the "32-bit domain ID problem" and the
pci-hyperv.c code was updated to avoid it.
I don't have information on whether user space code has been fixed, or
the extent to which such a fix has propagated into distro versions. At the
least, a VM with old user space code might break if the kernel is upgraded
to one with this patch. How do people see the risks now that it is 6 years
later? I don't have enough data to make an assessment.
Michael
> + return ida_alloc_range(&pci_domain_nr_dynamic_ida, 0x10000, INT_MAX,
> + GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(pci_bus_find_emul_domain_nr);
> +
> +void pci_bus_release_emul_domain_nr(int domain_nr)
> +{
> + ida_free(&pci_domain_nr_dynamic_ida, domain_nr);
> +}
> +EXPORT_SYMBOL_GPL(pci_bus_release_emul_domain_nr);
> +#endif
> +
> #ifdef CONFIG_PCI_DOMAINS_GENERIC
> static DEFINE_IDA(pci_domain_nr_static_ida);
> -static DEFINE_IDA(pci_domain_nr_dynamic_ida);
>
> static void of_pci_reserve_static_domain_nr(void)
> {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..e94978c3be3d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -632,6 +632,11 @@ static void pci_release_host_bridge_dev(struct device *dev)
>
> pci_free_resource_list(&bridge->windows);
> pci_free_resource_list(&bridge->dma_ranges);
> +
> + /* Host bridges only have domain_nr set in the emulation case */
> + if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
> + pci_bus_release_emul_domain_nr(bridge->domain_nr);
> +
> kfree(bridge);
> }
>
> @@ -1112,7 +1117,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> device_del(&bridge->dev);
> free:
> #ifdef CONFIG_PCI_DOMAINS_GENERIC
> - pci_bus_release_domain_nr(parent, bus->domain_nr);
> + if (bridge->domain_nr == PCI_DOMAIN_NR_NOT_SET)
> + pci_bus_release_domain_nr(parent, bus->domain_nr);
> #endif
> if (bus_registered)
> put_device(&bus->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..f6a713da5c49 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1934,10 +1934,14 @@ DEFINE_GUARD(pci_dev, struct pci_dev *,
> pci_dev_lock(_T), pci_dev_unlock(_T))
> */
> #ifdef CONFIG_PCI_DOMAINS
> extern int pci_domains_supported;
> +int pci_bus_find_emul_domain_nr(int hint);
> +void pci_bus_release_emul_domain_nr(int domain_nr);
> #else
> enum { pci_domains_supported = 0 };
> static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
> static inline int pci_proc_domain(struct pci_bus *bus) { return 0; }
> +static inline int pci_bus_find_emul_domain_nr(int hint) { return 0; }
> +static inline void pci_bus_release_emul_domain_nr(int domain_nr) { }
> #endif /* CONFIG_PCI_DOMAINS */
>
> /*
> --
> 2.50.1
>
^ permalink raw reply
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: dan.j.williams @ 2025-07-17 19:59 UTC (permalink / raw)
To: Michael Kelley, Dan Williams, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Manivannan Sadhasivam,
Rob Herring, K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <SN6PR02MB4157ADD06608EFE00B86A3F7D451A@SN6PR02MB4157.namprd02.prod.outlook.com>
Michael Kelley wrote:
> From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, July 16, 2025 9:09 AM
Thanks for taking a look Michael!
[..]
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index e9448d55113b..833ebf2d5213 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6692,9 +6692,50 @@ static void pci_no_domains(void)
> > #endif
> > }
> >
> > +#ifdef CONFIG_PCI_DOMAINS
> > +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > +
> > +/*
> > + * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > + * fallback to the first free domain number above the last ACPI segment number.
> > + * Caller may have a specific domain number in mind, in which case try to
> > + * reserve it.
> > + *
> > + * Note that this allocation is freed by pci_release_host_bridge_dev().
> > + */
> > +int pci_bus_find_emul_domain_nr(int hint)
> > +{
> > + if (hint >= 0) {
> > + hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > + GFP_KERNEL);
>
> This almost preserves the existing functionality in pci-hyperv.c. But if the
> "hint" passed in is zero, current code in pci-hyperv.c treats that as a
> collision and allocates some other value. The special treatment of zero is
> necessary per the comment with the definition of HVPCI_DOM_INVALID.
>
> I don't have an opinion on whether the code here should treat a "hint"
> of zero as invalid, or whether that should be handled in pci-hyperv.c.
Oh, I see what you are saying. I made the "hint == 0" case start working
where previously it should have failed. I feel like that's probably best
handled in pci-hyperv.c with something like the following which also
fixes up a regression I caused with @dom being unsigned:
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index cfe9806bdbe4..813757db98d1 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
{
struct pci_host_bridge *bridge;
struct hv_pcibus_device *hbus;
- u16 dom_req, dom;
+ int ret, dom = -EINVAL;
+ u16 dom_req;
char *name;
- int ret;
bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
if (!bridge)
@@ -3673,7 +3673,8 @@ static int hv_pci_probe(struct hv_device *hdev,
* collisions) in the same VM.
*/
dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
- dom = pci_bus_find_emul_domain_nr(dom_req);
+ if (dom_req)
+ dom = pci_bus_find_emul_domain_nr(dom_req);
if (dom < 0) {
dev_err(&hdev->device,
> > +
> > + if (hint >= 0)
> > + return hint;
> > + }
> > +
> > + if (acpi_disabled)
> > + return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> > +
> > + /*
> > + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> > + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> > + * which the lower 16 bits are the PCI Segment Group (domain) number.
> > + * Other bits are currently reserved.
> > + */
>
> Back in 2018 and 2019, the Microsoft Linux team encountered problems with
> PCI domain IDs that exceeded 0xFFFF. User space code, such as the Xorg X server,
> assumed PCI domain IDs were at most 16 bits, and retained only the low 16 bits
> if the value was larger. My memory of the details is vague, but I believe some
> or all of this behavior was tied to libpciaccess. As a result of these user space
> limitations, the pci-hyperv.c code made sure to not create any domain IDs
> larger than 0xFFFF. The problem was not just theoretical -- Microsoft had
> customers reporting issues due to the "32-bit domain ID problem" and the
> pci-hyperv.c code was updated to avoid it.
>
> I don't have information on whether user space code has been fixed, or
> the extent to which such a fix has propagated into distro versions. At the
> least, a VM with old user space code might break if the kernel is upgraded
> to one with this patch. How do people see the risks now that it is 6 years
> later? I don't have enough data to make an assessment.
A couple observations:
- I think it would be reasonable to not fallback in the hint case with
something like this:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 833ebf2d5213..0bd2053dbe8a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6705,14 +6705,10 @@ static DEFINE_IDA(pci_domain_nr_dynamic_ida);
*/
int pci_bus_find_emul_domain_nr(int hint)
{
- if (hint >= 0) {
- hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
+ if (hint >= 0)
+ return ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
GFP_KERNEL);
- if (hint >= 0)
- return hint;
- }
-
if (acpi_disabled)
return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
- The VMD driver has been allocating 32-bit PCI domain numbers since
v4.5 185a383ada2e ("x86/PCI: Add driver for Intel Volume Management
Device (VMD)"). At a minimum if it is still a problem, it is a shared
problem, but the significant deployment of VMD in the time likely
indicates it is ok. If not, the above change at least makes the
hyper-v case avoid 32-bit domain numbers.
^ permalink raw reply related
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: Michael Kelley @ 2025-07-17 23:06 UTC (permalink / raw)
To: dan.j.williams@intel.com, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Rob Herring,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <68795609847f7_137e6b100d8@dwillia2-xfh.jf.intel.com.notmuch>
From: dan.j.williams@intel.com <dan.j.williams@intel.com> Sent: Thursday, July 17, 2025 12:59 PM
>
> Michael Kelley wrote:
> > From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, July 16, 2025 9:09 AM
>
> Thanks for taking a look Michael!
>
> [..]
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index e9448d55113b..833ebf2d5213 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -6692,9 +6692,50 @@ static void pci_no_domains(void)
> > > #endif
> > > }
> > >
> > > +#ifdef CONFIG_PCI_DOMAINS
> > > +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > > +
> > > +/*
> > > + * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > > + * fallback to the first free domain number above the last ACPI segment number.
> > > + * Caller may have a specific domain number in mind, in which case try to
> > > + * reserve it.
> > > + *
> > > + * Note that this allocation is freed by pci_release_host_bridge_dev().
> > > + */
> > > +int pci_bus_find_emul_domain_nr(int hint)
> > > +{
> > > + if (hint >= 0) {
> > > + hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > > + GFP_KERNEL);
> >
> > This almost preserves the existing functionality in pci-hyperv.c. But if the
> > "hint" passed in is zero, current code in pci-hyperv.c treats that as a
> > collision and allocates some other value. The special treatment of zero is
> > necessary per the comment with the definition of HVPCI_DOM_INVALID.
> >
> > I don't have an opinion on whether the code here should treat a "hint"
> > of zero as invalid, or whether that should be handled in pci-hyperv.c.
>
> Oh, I see what you are saying. I made the "hint == 0" case start working
> where previously it should have failed. I feel like that's probably best
> handled in pci-hyperv.c with something like the following which also
> fixes up a regression I caused with @dom being unsigned:
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index cfe9806bdbe4..813757db98d1 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> {
> struct pci_host_bridge *bridge;
> struct hv_pcibus_device *hbus;
> - u16 dom_req, dom;
> + int ret, dom = -EINVAL;
> + u16 dom_req;
> char *name;
> - int ret;
>
> bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> if (!bridge)
> @@ -3673,7 +3673,8 @@ static int hv_pci_probe(struct hv_device *hdev,
> * collisions) in the same VM.
> */
> dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> - dom = pci_bus_find_emul_domain_nr(dom_req);
> + if (dom_req)
> + dom = pci_bus_find_emul_domain_nr(dom_req);
No, I don't think this is right either. If dom_req is 0, we don't want to
hv_pci_probe() to fail. We want the "collision" path to be taken so that
some other unused PCI domain ID is assigned. That could be done by
passing -1 as the hint to pci_bus_bind_emul_domain_nr(). Or PCI
domain ID 0 could be pre-reserved in init_hv_pci_drv() like is done
with HVPCI_DOM_INVALID in current code.
>
> if (dom < 0) {
> dev_err(&hdev->device,
>
> > > +
> > > + if (hint >= 0)
> > > + return hint;
> > > + }
> > > +
> > > + if (acpi_disabled)
> > > + return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> > > +
> > > + /*
> > > + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> > > + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> > > + * which the lower 16 bits are the PCI Segment Group (domain) number.
> > > + * Other bits are currently reserved.
> > > + */
> >
> > Back in 2018 and 2019, the Microsoft Linux team encountered problems with
> > PCI domain IDs that exceeded 0xFFFF. User space code, such as the Xorg X server,
> > assumed PCI domain IDs were at most 16 bits, and retained only the low 16 bits
> > if the value was larger. My memory of the details is vague, but I believe some
> > or all of this behavior was tied to libpciaccess. As a result of these user space
> > limitations, the pci-hyperv.c code made sure to not create any domain IDs
> > larger than 0xFFFF. The problem was not just theoretical -- Microsoft had
> > customers reporting issues due to the "32-bit domain ID problem" and the
> > pci-hyperv.c code was updated to avoid it.
> >
> > I don't have information on whether user space code has been fixed, or
> > the extent to which such a fix has propagated into distro versions. At the
> > least, a VM with old user space code might break if the kernel is upgraded
> > to one with this patch. How do people see the risks now that it is 6 years
> > later? I don't have enough data to make an assessment.
>
> A couple observations:
>
> - I think it would be reasonable to not fallback in the hint case with
> something like this:
We *do* need the fallback in the hint case. If the hint causes a collision
(i.e., another device is already using the hinted PCI domain ID), then we
need to choose some other PCI domain ID. Again, we don't want hv_pci_probe()
to fail for the device because the value of bytes 4 and 5 chosen from device's
GUID (as assigned by Hyper-V) accidently matches bytes 4 and 5 of some other
device's GUID. Hyper-V guarantees the GUIDs are unique, but not bytes 4 and
5 standing alone. Current code behaves like the acpi_disabled case in your
patch, and picks some other unused PCI domain ID in the 1 to 0xFFFF range.
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 833ebf2d5213..0bd2053dbe8a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6705,14 +6705,10 @@ static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> */
> int pci_bus_find_emul_domain_nr(int hint)
> {
> - if (hint >= 0) {
> - hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> + if (hint >= 0)
> + return ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> GFP_KERNEL);
>
> - if (hint >= 0)
> - return hint;
> - }
> -
> if (acpi_disabled)
> return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
>
> - The VMD driver has been allocating 32-bit PCI domain numbers since
> v4.5 185a383ada2e ("x86/PCI: Add driver for Intel Volume Management
> Device (VMD)"). At a minimum if it is still a problem, it is a shared
> problem, but the significant deployment of VMD in the time likely
> indicates it is ok. If not, the above change at least makes the
> hyper-v case avoid 32-bit domain numbers.
The problem we encountered in 2018/2019 was with graphics devices
and the Xorg X Server, specifically with the PCI domain ID stored in
xorg.conf to identify the graphics device that the X Server was to run
against. I don't recall ever seeing a similar problem with storage or NIC
devices, but my memory could be incomplete. It's plausible that user
space code accessing the VMD device correctly handled 32-bit domain
IDs, but that's not necessarily an indicator for user space graphics
software. The Xorg X Server issues would have started somewhere after
commit 4a9b0933bdfc in the 4.11 kernel, and were finally fixed in the 5.4
kernel with commits be700103efd10 and f73f8a504e279.
All that said, I'm not personally averse to trying again in assigning a
domain ID > 0xFFFF. I do see a commit [1] to fix libpciaccess that was
made 7 years ago in response to the issues we were seeing on Hyper-V.
Assuming those fixes have propagated into using packages like X Server,
then we're good. But someone from Microsoft should probably sign off
on taking this risk. I retired from Microsoft nearly two years ago, and
meddle in things from time-to-time without the burden of dealing
with customer support issues. ;-)
[1] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/commit/a167bd6474522a709ff3cbb00476c0e4309cb66f
^ permalink raw reply
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: dan.j.williams @ 2025-07-18 0:22 UTC (permalink / raw)
To: Michael Kelley, dan.j.williams@intel.com, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Rob Herring,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <SN6PR02MB4157E31D8448CD9D81D518C5D451A@SN6PR02MB4157.namprd02.prod.outlook.com>
Michael Kelley wrote:
> From: dan.j.williams@intel.com <dan.j.williams@intel.com> Sent: Thursday, July 17, 2025 12:59 PM
> >
> > Michael Kelley wrote:
> > > From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, July 16, 2025 9:09 AM
> >
> > Thanks for taking a look Michael!
> >
> > [..]
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index e9448d55113b..833ebf2d5213 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -6692,9 +6692,50 @@ static void pci_no_domains(void)
> > > > #endif
> > > > }
> > > >
> > > > +#ifdef CONFIG_PCI_DOMAINS
> > > > +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > > > +
> > > > +/*
> > > > + * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > > > + * fallback to the first free domain number above the last ACPI segment number.
> > > > + * Caller may have a specific domain number in mind, in which case try to
> > > > + * reserve it.
> > > > + *
> > > > + * Note that this allocation is freed by pci_release_host_bridge_dev().
> > > > + */
> > > > +int pci_bus_find_emul_domain_nr(int hint)
> > > > +{
> > > > + if (hint >= 0) {
> > > > + hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > > > + GFP_KERNEL);
> > >
> > > This almost preserves the existing functionality in pci-hyperv.c. But if the
> > > "hint" passed in is zero, current code in pci-hyperv.c treats that as a
> > > collision and allocates some other value. The special treatment of zero is
> > > necessary per the comment with the definition of HVPCI_DOM_INVALID.
> > >
> > > I don't have an opinion on whether the code here should treat a "hint"
> > > of zero as invalid, or whether that should be handled in pci-hyperv.c.
> >
> > Oh, I see what you are saying. I made the "hint == 0" case start working
> > where previously it should have failed. I feel like that's probably best
> > handled in pci-hyperv.c with something like the following which also
> > fixes up a regression I caused with @dom being unsigned:
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index cfe9806bdbe4..813757db98d1 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> > {
> > struct pci_host_bridge *bridge;
> > struct hv_pcibus_device *hbus;
> > - u16 dom_req, dom;
> > + int ret, dom = -EINVAL;
> > + u16 dom_req;
> > char *name;
> > - int ret;
> >
> > bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> > if (!bridge)
> > @@ -3673,7 +3673,8 @@ static int hv_pci_probe(struct hv_device *hdev,
> > * collisions) in the same VM.
> > */
> > dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> > - dom = pci_bus_find_emul_domain_nr(dom_req);
> > + if (dom_req)
> > + dom = pci_bus_find_emul_domain_nr(dom_req);
>
> No, I don't think this is right either. If dom_req is 0, we don't want to
> hv_pci_probe() to fail. We want the "collision" path to be taken so that
> some other unused PCI domain ID is assigned. That could be done by
> passing -1 as the hint to pci_bus_bind_emul_domain_nr(). Or PCI
> domain ID 0 could be pre-reserved in init_hv_pci_drv() like is done
> with HVPCI_DOM_INVALID in current code.
Yeah, I realized that shortly after sending. I will slow down.
> >
> > A couple observations:
> >
> > - I think it would be reasonable to not fallback in the hint case with
> > something like this:
>
> We *do* need the fallback in the hint case. If the hint causes a collision
> (i.e., another device is already using the hinted PCI domain ID), then we
> need to choose some other PCI domain ID. Again, we don't want hv_pci_probe()
> to fail for the device because the value of bytes 4 and 5 chosen from device's
> GUID (as assigned by Hyper-V) accidently matches bytes 4 and 5 of some other
> device's GUID. Hyper-V guarantees the GUIDs are unique, but not bytes 4 and
> 5 standing alone. Current code behaves like the acpi_disabled case in your
> patch, and picks some other unused PCI domain ID in the 1 to 0xFFFF range.
Ok, that feels like "let the caller set the range in addition to the
hint".
>
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 833ebf2d5213..0bd2053dbe8a 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6705,14 +6705,10 @@ static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > */
> > int pci_bus_find_emul_domain_nr(int hint)
> > {
> > - if (hint >= 0) {
> > - hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > + if (hint >= 0)
> > + return ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > GFP_KERNEL);
> >
> > - if (hint >= 0)
> > - return hint;
> > - }
> > -
> > if (acpi_disabled)
> > return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> >
> > - The VMD driver has been allocating 32-bit PCI domain numbers since
> > v4.5 185a383ada2e ("x86/PCI: Add driver for Intel Volume Management
> > Device (VMD)"). At a minimum if it is still a problem, it is a shared
> > problem, but the significant deployment of VMD in the time likely
> > indicates it is ok. If not, the above change at least makes the
> > hyper-v case avoid 32-bit domain numbers.
>
> The problem we encountered in 2018/2019 was with graphics devices
> and the Xorg X Server, specifically with the PCI domain ID stored in
> xorg.conf to identify the graphics device that the X Server was to run
> against. I don't recall ever seeing a similar problem with storage or NIC
> devices, but my memory could be incomplete. It's plausible that user
> space code accessing the VMD device correctly handled 32-bit domain
> IDs, but that's not necessarily an indicator for user space graphics
> software. The Xorg X Server issues would have started somewhere after
> commit 4a9b0933bdfc in the 4.11 kernel, and were finally fixed in the 5.4
> kernel with commits be700103efd10 and f73f8a504e279.
>
> All that said, I'm not personally averse to trying again in assigning a
> domain ID > 0xFFFF. I do see a commit [1] to fix libpciaccess that was
> made 7 years ago in response to the issues we were seeing on Hyper-V.
> Assuming those fixes have propagated into using packages like X Server,
> then we're good. But someone from Microsoft should probably sign off
> on taking this risk. I retired from Microsoft nearly two years ago, and
> meddle in things from time-to-time without the burden of dealing
> with customer support issues. ;-)
Living the dream! Extra thanks for taking a look.
> [1] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/commit/a167bd6474522a709ff3cbb00476c0e4309cb66f
Thanks for this.
I would rather do the equivalent conversion for now because 7 years old
is right on the cusp of "someone might still be running that with new
kernels".
Here is the replacement fixup that I will fold in if it looks good to
you:
-- 8< --
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index cfe9806bdbe4..f1079a438bff 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
{
struct pci_host_bridge *bridge;
struct hv_pcibus_device *hbus;
- u16 dom_req, dom;
+ int ret, dom;
+ u16 dom_req;
char *name;
- int ret;
bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
if (!bridge)
@@ -3673,8 +3673,7 @@ static int hv_pci_probe(struct hv_device *hdev,
* collisions) in the same VM.
*/
dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
- dom = pci_bus_find_emul_domain_nr(dom_req);
-
+ dom = pci_bus_find_emul_domain_nr(dom_req, 1, U16_MAX);
if (dom < 0) {
dev_err(&hdev->device,
"Unable to use dom# 0x%x or other numbers", dom_req);
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index f60244ff9ef8..30935fe85af9 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -881,7 +881,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
sd->vmd_dev = vmd->dev;
- sd->domain = pci_bus_find_emul_domain_nr(PCI_DOMAIN_NR_NOT_SET);
+
+ /*
+ * Emulated domains start at 0x10000 to not clash with ACPI _SEG
+ * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
+ * which the lower 16 bits are the PCI Segment Group (domain) number.
+ * Other bits are currently reserved.
+ */
+ sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
if (sd->domain < 0)
return sd->domain;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 833ebf2d5213..de42e53f07d0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6695,34 +6695,15 @@ static void pci_no_domains(void)
#ifdef CONFIG_PCI_DOMAINS
static DEFINE_IDA(pci_domain_nr_dynamic_ida);
-/*
- * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
- * fallback to the first free domain number above the last ACPI segment number.
- * Caller may have a specific domain number in mind, in which case try to
- * reserve it.
- *
- * Note that this allocation is freed by pci_release_host_bridge_dev().
+/**
+ * pci_bus_find_emul_domain_nr() - allocate a PCI domain number per constraints
+ * @hint: desired domain, 0 if any id in the range of @min to @max is acceptable
+ * @min: minimum allowable domain
+ * @max: maximum allowable domain, no ids higher than INT_MAX will be returned
*/
-int pci_bus_find_emul_domain_nr(int hint)
+u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
{
- if (hint >= 0) {
- hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
- GFP_KERNEL);
-
- if (hint >= 0)
- return hint;
- }
-
- if (acpi_disabled)
- return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
-
- /*
- * Emulated domains start at 0x10000 to not clash with ACPI _SEG
- * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
- * which the lower 16 bits are the PCI Segment Group (domain) number.
- * Other bits are currently reserved.
- */
- return ida_alloc_range(&pci_domain_nr_dynamic_ida, 0x10000, INT_MAX,
+ return ida_alloc_range(&pci_domain_nr_dynamic_ida, max(hint, min), max,
GFP_KERNEL);
}
EXPORT_SYMBOL_GPL(pci_bus_find_emul_domain_nr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f6a713da5c49..4aeabe8e2f1f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1934,13 +1934,16 @@ DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
*/
#ifdef CONFIG_PCI_DOMAINS
extern int pci_domains_supported;
-int pci_bus_find_emul_domain_nr(int hint);
+u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max);
void pci_bus_release_emul_domain_nr(int domain_nr);
#else
enum { pci_domains_supported = 0 };
static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
static inline int pci_proc_domain(struct pci_bus *bus) { return 0; }
-static inline int pci_bus_find_emul_domain_nr(int hint) { return 0; }
+static inline u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
+{
+ return 0;
+}
static inline void pci_bus_release_emul_domain_nr(int domain_nr) { }
#endif /* CONFIG_PCI_DOMAINS */
^ permalink raw reply related
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: Michael Kelley @ 2025-07-18 3:03 UTC (permalink / raw)
To: dan.j.williams@intel.com, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Rob Herring,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <687993e03bb4c_14c3561008c@dwillia2-xfh.jf.intel.com.notmuch>
From: dan.j.williams@intel.com <dan.j.williams@intel.com> Sent: Thursday, July 17, 2025 5:23 PM
>
> Michael Kelley wrote:
> > From: dan.j.williams@intel.com <dan.j.williams@intel.com> Sent: Thursday, July 17, 2025 12:59 PM
> > >
> > > Michael Kelley wrote:
> > > > From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, July 16, 2025 9:09 AM
> > >
> > > Thanks for taking a look Michael!
> > >
> > > [..]
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index e9448d55113b..833ebf2d5213 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -6692,9 +6692,50 @@ static void pci_no_domains(void)
> > > > > #endif
> > > > > }
> > > > >
> > > > > +#ifdef CONFIG_PCI_DOMAINS
> > > > > +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > > > > +
> > > > > +/*
> > > > > + * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > > > > + * fallback to the first free domain number above the last ACPI segment number.
> > > > > + * Caller may have a specific domain number in mind, in which case try to
> > > > > + * reserve it.
> > > > > + *
> > > > > + * Note that this allocation is freed by pci_release_host_bridge_dev().
> > > > > + */
> > > > > +int pci_bus_find_emul_domain_nr(int hint)
> > > > > +{
> > > > > + if (hint >= 0) {
> > > > > + hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > > > > + GFP_KERNEL);
> > > >
> > > > This almost preserves the existing functionality in pci-hyperv.c. But if the
> > > > "hint" passed in is zero, current code in pci-hyperv.c treats that as a
> > > > collision and allocates some other value. The special treatment of zero is
> > > > necessary per the comment with the definition of HVPCI_DOM_INVALID.
> > > >
> > > > I don't have an opinion on whether the code here should treat a "hint"
> > > > of zero as invalid, or whether that should be handled in pci-hyperv.c.
> > >
> > > Oh, I see what you are saying. I made the "hint == 0" case start working
> > > where previously it should have failed. I feel like that's probably best
> > > handled in pci-hyperv.c with something like the following which also
> > > fixes up a regression I caused with @dom being unsigned:
> > >
> > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > > index cfe9806bdbe4..813757db98d1 100644
> > > --- a/drivers/pci/controller/pci-hyperv.c
> > > +++ b/drivers/pci/controller/pci-hyperv.c
> > > @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> > > {
> > > struct pci_host_bridge *bridge;
> > > struct hv_pcibus_device *hbus;
> > > - u16 dom_req, dom;
> > > + int ret, dom = -EINVAL;
> > > + u16 dom_req;
> > > char *name;
> > > - int ret;
> > >
> > > bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> > > if (!bridge)
> > > @@ -3673,7 +3673,8 @@ static int hv_pci_probe(struct hv_device *hdev,
> > > * collisions) in the same VM.
> > > */
> > > dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> > > - dom = pci_bus_find_emul_domain_nr(dom_req);
> > > + if (dom_req)
> > > + dom = pci_bus_find_emul_domain_nr(dom_req);
> >
> > No, I don't think this is right either. If dom_req is 0, we don't want to
> > hv_pci_probe() to fail. We want the "collision" path to be taken so that
> > some other unused PCI domain ID is assigned. That could be done by
> > passing -1 as the hint to pci_bus_bind_emul_domain_nr(). Or PCI
> > domain ID 0 could be pre-reserved in init_hv_pci_drv() like is done
> > with HVPCI_DOM_INVALID in current code.
>
> Yeah, I realized that shortly after sending. I will slow down.
>
> > >
> > > A couple observations:
> > >
> > > - I think it would be reasonable to not fallback in the hint case with
> > > something like this:
> >
> > We *do* need the fallback in the hint case. If the hint causes a collision
> > (i.e., another device is already using the hinted PCI domain ID), then we
> > need to choose some other PCI domain ID. Again, we don't want hv_pci_probe()
> > to fail for the device because the value of bytes 4 and 5 chosen from device's
> > GUID (as assigned by Hyper-V) accidently matches bytes 4 and 5 of some other
> > device's GUID. Hyper-V guarantees the GUIDs are unique, but not bytes 4 and
> > 5 standing alone. Current code behaves like the acpi_disabled case in your
> > patch, and picks some other unused PCI domain ID in the 1 to 0xFFFF range.
>
> Ok, that feels like "let the caller set the range in addition to the
> hint".
>
> >
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 833ebf2d5213..0bd2053dbe8a 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -6705,14 +6705,10 @@ static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > > */
> > > int pci_bus_find_emul_domain_nr(int hint)
> > > {
> > > - if (hint >= 0) {
> > > - hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > > + if (hint >= 0)
> > > + return ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> > > GFP_KERNEL);
> > >
> > > - if (hint >= 0)
> > > - return hint;
> > > - }
> > > -
> > > if (acpi_disabled)
> > > return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> > >
> > > - The VMD driver has been allocating 32-bit PCI domain numbers since
> > > v4.5 185a383ada2e ("x86/PCI: Add driver for Intel Volume Management
> > > Device (VMD)"). At a minimum if it is still a problem, it is a shared
> > > problem, but the significant deployment of VMD in the time likely
> > > indicates it is ok. If not, the above change at least makes the
> > > hyper-v case avoid 32-bit domain numbers.
> >
> > The problem we encountered in 2018/2019 was with graphics devices
> > and the Xorg X Server, specifically with the PCI domain ID stored in
> > xorg.conf to identify the graphics device that the X Server was to run
> > against. I don't recall ever seeing a similar problem with storage or NIC
> > devices, but my memory could be incomplete. It's plausible that user
> > space code accessing the VMD device correctly handled 32-bit domain
> > IDs, but that's not necessarily an indicator for user space graphics
> > software. The Xorg X Server issues would have started somewhere after
> > commit 4a9b0933bdfc in the 4.11 kernel, and were finally fixed in the 5.4
> > kernel with commits be700103efd10 and f73f8a504e279.
> >
> > All that said, I'm not personally averse to trying again in assigning a
> > domain ID > 0xFFFF. I do see a commit [1] to fix libpciaccess that was
> > made 7 years ago in response to the issues we were seeing on Hyper-V.
> > Assuming those fixes have propagated into using packages like X Server,
> > then we're good. But someone from Microsoft should probably sign off
> > on taking this risk. I retired from Microsoft nearly two years ago, and
> > meddle in things from time-to-time without the burden of dealing
> > with customer support issues. ;-)
>
> Living the dream! Extra thanks for taking a look.
>
> > [1] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/commit/a167bd6474522a709ff3cbb00476c0e4309cb66f >
> Thanks for this.
>
> I would rather do the equivalent conversion for now because 7 years old
> is right on the cusp of "someone might still be running that with new
> kernels".
Works for me, and is a bit less risky.
>
> Here is the replacement fixup that I will fold in if it looks good to
> you:
>
> -- 8< --
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index cfe9806bdbe4..f1079a438bff 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> {
> struct pci_host_bridge *bridge;
> struct hv_pcibus_device *hbus;
> - u16 dom_req, dom;
> + int ret, dom;
> + u16 dom_req;
> char *name;
> - int ret;
>
> bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> if (!bridge)
> @@ -3673,8 +3673,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> * collisions) in the same VM.
> */
> dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> - dom = pci_bus_find_emul_domain_nr(dom_req);
> -
As an additional paragraph the larger comment block above, let's include a
massaged version of the comment associated with HVPCI_DOM_INVALID.
Perhaps:
*
* Because Gen1 VMs use domain 0, don't allow picking domain 0 here, even
* if bytes 4 and 5 of the instance GUID are both zero.
*/
> + dom = pci_bus_find_emul_domain_nr(dom_req, 1, U16_MAX);
> if (dom < 0) {
> dev_err(&hdev->device,
> "Unable to use dom# 0x%x or other numbers", dom_req);
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index f60244ff9ef8..30935fe85af9 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -881,7 +881,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
>
> sd->vmd_dev = vmd->dev;
> - sd->domain = pci_bus_find_emul_domain_nr(PCI_DOMAIN_NR_NOT_SET);
> +
> + /*
> + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> + * which the lower 16 bits are the PCI Segment Group (domain) number.
> + * Other bits are currently reserved.
> + */
> + sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
> if (sd->domain < 0)
> return sd->domain;
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 833ebf2d5213..de42e53f07d0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6695,34 +6695,15 @@ static void pci_no_domains(void)
> #ifdef CONFIG_PCI_DOMAINS
> static DEFINE_IDA(pci_domain_nr_dynamic_ida);
>
> -/*
> - * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> - * fallback to the first free domain number above the last ACPI segment number.
> - * Caller may have a specific domain number in mind, in which case try to
> - * reserve it.
> - *
> - * Note that this allocation is freed by pci_release_host_bridge_dev().
> +/**
> + * pci_bus_find_emul_domain_nr() - allocate a PCI domain number per constraints
> + * @hint: desired domain, 0 if any id in the range of @min to @max is acceptable
> + * @min: minimum allowable domain
> + * @max: maximum allowable domain, no ids higher than INT_MAX will be returned
> */
> -int pci_bus_find_emul_domain_nr(int hint)
> +u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
Shouldn't the return type here still be "int"? ida_alloc_range() can return a negative
errno if it fails. And the call sites in hv_pci_probe() and vmd_enable_domain()
store the return value into an "int".
Other than that, and my suggested added comment, this looks good.
Michael
> {
> - if (hint >= 0) {
> - hint = ida_alloc_range(&pci_domain_nr_dynamic_ida, hint, hint,
> - GFP_KERNEL);
> -
> - if (hint >= 0)
> - return hint;
> - }
> -
> - if (acpi_disabled)
> - return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
> -
> - /*
> - * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> - * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> - * which the lower 16 bits are the PCI Segment Group (domain) number.
> - * Other bits are currently reserved.
> - */
> - return ida_alloc_range(&pci_domain_nr_dynamic_ida, 0x10000, INT_MAX,
> + return ida_alloc_range(&pci_domain_nr_dynamic_ida, max(hint, min), max,
> GFP_KERNEL);
> }
> EXPORT_SYMBOL_GPL(pci_bus_find_emul_domain_nr);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f6a713da5c49..4aeabe8e2f1f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1934,13 +1934,16 @@ DEFINE_GUARD(pci_dev, struct pci_dev *,
> pci_dev_lock(_T), pci_dev_unlock(_T))
> */
> #ifdef CONFIG_PCI_DOMAINS
> extern int pci_domains_supported;
> -int pci_bus_find_emul_domain_nr(int hint);
> +u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max);
> void pci_bus_release_emul_domain_nr(int domain_nr);
> #else
> enum { pci_domains_supported = 0 };
> static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
> static inline int pci_proc_domain(struct pci_bus *bus) { return 0; }
> -static inline int pci_bus_find_emul_domain_nr(int hint) { return 0; }
> +static inline u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
> +{
> + return 0;
> +}
> static inline void pci_bus_release_emul_domain_nr(int domain_nr) { }
> #endif /* CONFIG_PCI_DOMAINS */
>
^ permalink raw reply
* Re: [PATCH v5 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Naman Jain @ 2025-07-18 4:36 UTC (permalink / raw)
To: Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui
Cc: Roman Kisel, Anirudh Rayabharam, Saurabh Sengar,
Stanislav Kinsburskii, Nuno Das Neves, ALOK TIWARI,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
In-Reply-To: <SN6PR02MB4157F9F1F8493C74C9FCC6E4D449A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 7/9/2025 10:49 PM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, June 11, 2025 12:27 AM
>>
>> Provide an interface for Virtual Machine Monitor like OpenVMM and its
>> use as OpenHCL paravisor to control VTL0 (Virtual trust Level).
>> Expose devices and support IOCTLs for features like VTL creation,
>> VTL0 memory management, context switch, making hypercalls,
>> mapping VTL0 address space to VTL2 userspace, getting new VMBus
>> messages and channel events in VTL2 etc.
>>
>
> I know it has been 4 weeks since you posted this patch, but I'm just
> now getting around to reviewing it. :-(
>
> I've reviewed most of it reasonably carefully, but there are some
> parts, such as entering/exiting VTLs that I don't know the details of,
> and so just glossed over.
>
>> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Reviewed-by: Roman Kisel <romank@linux.microsoft.com>
>> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
>> Message-ID: <20250512140432.2387503-3-namjain@linux.microsoft.com>
>> Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>> drivers/hv/Kconfig | 23 +
>> drivers/hv/Makefile | 7 +-
>> drivers/hv/mshv_vtl.h | 52 +
>> drivers/hv/mshv_vtl_main.c | 1783 +++++++++++++++++++++++++++++++++++
>> include/hyperv/hvgdk_mini.h | 81 ++
>> include/hyperv/hvhdk.h | 1 +
>> include/uapi/linux/mshv.h | 82 ++
>> 7 files changed, 2028 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/hv/mshv_vtl.h
>> create mode 100644 drivers/hv/mshv_vtl_main.c
>>
>> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
>> index 1cd188b73b74..1403b4abbece 100644
>> --- a/drivers/hv/Kconfig
>> +++ b/drivers/hv/Kconfig
>> @@ -73,4 +73,27 @@ config MSHV_ROOT
>>
>> If unsure, say N.
>>
>> +config MSHV_VTL
>> + tristate "Microsoft Hyper-V VTL driver"
>> + depends on HYPERV && X86_64
>> + # Mapping VTL0 memory to a userspace process in VTL2 is supported in OpenHCL.
>> + # VTL2 for OpenHCL makes use of Huge Pages to improve performance on VMs,
>> + # specially with large memory requirements.
>> + depends on TRANSPARENT_HUGEPAGE
>> + # MTRRs are controlled by VTL0, and are not specific to individual VTLs.
>> + # Therefore, do not attempt to access or modify MTRRs here.
>> + depends on !MTRR
>> + select CPUMASK_OFFSTACK
>> + select HYPERV_VTL_MODE
>
> I think "depends on HYPERV_VTL_MODE" would work better here. Kconfig
> allows you to "select" something that has dependencies that aren't satisfied,
> with the result that the kernel probably won't build. HYPERV_VTL_MODE
> has dependencies like "SMP" that you would need to duplicate here. If
> you change this to "depends on", then you don't need "depends on HYPERV"
> because that's covered by HYPERV_VTL_MODE. It looks like for now this
> is restricted to X86_64, though I'm guessing at some point in the future that
> restriction will go away.
Will address in next patch.
>
>> + default n
>> + help
>> + Select this option to enable Hyper-V VTL driver support.
>> + This driver provides interfaces for Virtual Machine Manager (VMM) running in VTL2
>> + userspace to create VTLs and partitions, setup and manage VTL0 memory and
>> + allow userspace to make direct hypercalls. This also allows to map VTL0's address
>> + space to a usermode process in VTL2 and supports getting new VMBus messages and channel
>> + events in VTL2.
>> +
>> + If unsure, say N.
>> +
>> endmenu
>> diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
>> index 976189c725dc..c53a0df746b7 100644
>> --- a/drivers/hv/Makefile
>> +++ b/drivers/hv/Makefile
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_HYPERV) += hv_vmbus.o
>> obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o
>> obj-$(CONFIG_HYPERV_BALLOON) += hv_balloon.o
>> obj-$(CONFIG_MSHV_ROOT) += mshv_root.o
>> +obj-$(CONFIG_MSHV_VTL) += mshv_vtl.o
>>
>> CFLAGS_hv_trace.o = -I$(src)
>> CFLAGS_hv_balloon.o = -I$(src)
>> @@ -14,7 +15,11 @@ hv_vmbus-$(CONFIG_HYPERV_TESTING) +=
>> hv_debugfs.o
>> hv_utils-y := hv_util.o hv_kvp.o hv_snapshot.o hv_utils_transport.o
>> mshv_root-y := mshv_root_main.o mshv_synic.o mshv_eventfd.o mshv_irq.o \
>> mshv_root_hv_call.o mshv_portid_table.o
>> +mshv_vtl-y := mshv_vtl_main.o
>>
>> # Code that must be built-in
>> obj-$(subst m,y,$(CONFIG_HYPERV)) += hv_common.o
>> -obj-$(subst m,y,$(CONFIG_MSHV_ROOT)) += hv_proc.o mshv_common.o
>> +obj-$(subst m,y,$(CONFIG_MSHV_ROOT)) += hv_proc.o
>> +ifneq ($(CONFIG_MSHV_ROOT) $(CONFIG_MSHV_VTL),)
>> + obj-y += mshv_common.o
>> +endif
>> diff --git a/drivers/hv/mshv_vtl.h b/drivers/hv/mshv_vtl.h
>> new file mode 100644
>> index 000000000000..f765fda3601b
>> --- /dev/null
>> +++ b/drivers/hv/mshv_vtl.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _MSHV_VTL_H
>> +#define _MSHV_VTL_H
>> +
>> +#include <linux/mshv.h>
>> +#include <linux/types.h>
>> +#include <asm/fpu/types.h>
>> +
>> +struct mshv_vtl_cpu_context {
>> + union {
>> + struct {
>> + u64 rax;
>> + u64 rcx;
>> + u64 rdx;
>> + u64 rbx;
>> + u64 cr2;
>> + u64 rbp;
>> + u64 rsi;
>> + u64 rdi;
>> + u64 r8;
>> + u64 r9;
>> + u64 r10;
>> + u64 r11;
>> + u64 r12;
>> + u64 r13;
>> + u64 r14;
>> + u64 r15;
>> + };
>> + u64 gp_regs[16];
>> + };
>> +
>> + struct fxregs_state fx_state;
>> +};
>> +
>> +struct mshv_vtl_run {
>> + u32 cancel;
>> + u32 vtl_ret_action_size;
>> + u32 pad[2];
>> + char exit_message[MSHV_MAX_RUN_MSG_SIZE];
>> + union {
>> + struct mshv_vtl_cpu_context cpu_context;
>> +
>> + /*
>> + * Reserving room for the cpu context to grow and to maintain compatibility
>> + * with user mode.
>> + */
>> + char reserved[1024];
>> + };
>> + char vtl_ret_actions[MSHV_MAX_RUN_MSG_SIZE];
>> +};
>> +
>> +#endif /* _MSHV_VTL_H */
>> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
>> new file mode 100644
>> index 000000000000..b1717b118772
>> --- /dev/null
>> +++ b/drivers/hv/mshv_vtl_main.c
>> @@ -0,0 +1,1783 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Microsoft Corporation.
>> + *
>> + * Author:
>> + * Roman Kisel <romank@linux.microsoft.com>
>> + * Saurabh Sengar <ssengar@linux.microsoft.com>
>> + * Naman Jain <namjain@linux.microsoft.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/anon_inodes.h>
>> +#include <linux/pfn_t.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/count_zeros.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/poll.h>
>> +#include <linux/file.h>
>> +#include <linux/vmalloc.h>
>> +#include <asm/debugreg.h>
>> +#include <asm/mshyperv.h>
>> +#include <trace/events/ipi.h>
>> +#include <uapi/asm/mtrr.h>
<snip>
>> +#include <uapi/linux/mshv.h>
>> +#include <hyperv/hvhdk.h>
>> +
>> +#include "../../kernel/fpu/legacy.h"
>> +#include "mshv.h"
>> +#include "mshv_vtl.h"
>> +#include "hyperv_vmbus.h"
>> +
>> +MODULE_AUTHOR("Microsoft");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Microsoft Hyper-V VTL Driver");
>> +
>> +#define MSHV_ENTRY_REASON_LOWER_VTL_CALL 0x1
>> +#define MSHV_ENTRY_REASON_INTERRUPT 0x2
>> +#define MSHV_ENTRY_REASON_INTERCEPT 0x3
>> +
>> +#define MAX_GUEST_MEM_SIZE BIT_ULL(40)
>
> This definition doesn't appear to be used anywhere. I was curious
> about what the units are, but with no usage, maybe that's moot.
Removed in next patch.
>
>> +#define MSHV_PG_OFF_CPU_MASK 0xFFFF
>> +#define MSHV_REAL_OFF_SHIFT 16
>
> The above two definitions are related. Perhaps define the CPU_MASK
> as "BIT_ULL(MSHV_REAL_OFF_SHIFT) - 1" so the relationship is
> explicit?
That also works, noted.
>
>> +#define MSHV_RUN_PAGE_OFFSET 0
>> +#define MSHV_REG_PAGE_OFFSET 1
>> +#define VTL2_VMBUS_SINT_INDEX 7
>> +
>> +static struct device *mem_dev;
>> +
>> +static struct tasklet_struct msg_dpc;
>> +static wait_queue_head_t fd_wait_queue;
>> +static bool has_message;
>> +static struct eventfd_ctx *flag_eventfds[HV_EVENT_FLAGS_COUNT];
>> +static DEFINE_MUTEX(flag_lock);
>> +static bool __read_mostly mshv_has_reg_page;
>> +
>> +struct mshv_vtl_hvcall_fd {
>> + u64 allow_bitmap[2 * PAGE_SIZE];
>
> Having PAGE_SIZE here begs the question of what happens on ARM64 if the
> VTL0 guest has a page size other than 4K? Does VTL2 always use 4K page
> size, or must the VTL2 page size match VTL0? The Kconfig file is currently
> set up with VTL2 supported only for x86, so the question is moot for the
> moment, but I'm thinking ahead.
>
VTL2 is always using 4K page size in both arm64 and x86, as per usecase.
However, having PAGE_SIZE is not necessary in allow_bitmap size. Will
remove it. More explanation in below comments.
> Separately, "allow_bitmap" size is 64K bytes, or 512K bits. Is that the
> correct size? From looking at mshv_vtl_hvcall_is_allowed(), I think this
> bitmap is indexed by the HV call code, which is a 16 bit value. So you
> only need 64K bits, and the size is too big by a factor of 8. In any case,
> it seems like the size should not be expressed in terms of PAGE_SIZE.
There are HVcall codes which are of type u16. So max(HVcall code) =
0xffff.
For every HVcall that needs to be allowed, we are saving HVcall code
info in a bitmap in below fashion:
if x = HVCall code and bitmap is an array of u64, of size
((0xffff/64=1023) + 1)
bitmap[x / 64] = (u64)1 << (x%64);
Later on in mshv_vtl_hvcall_is_allowed(), we calculate the array index
by dividing it by 64, and then see if call_code/64 bit is set.
Coming to size of allow_bitmap[], it is independent of PAGE_SIZE, and
can be safely initialized to 1024 (reducing by a factor of 8).
bitmap_size's maximum value is going to be 1024 in current
implementation, picking u64 was not mandatory, u16 will also work. Also,
item_index is also u16, so I should make bitmap_size as u16.
>
>> + bool allow_map_initialized;
>> + /*
>> + * Used to protect hvcall setup in IOCTLs
>> + */
>> + struct mutex init_mutex;
>> + struct miscdevice *dev;
>> +};
>> +
>> +struct mshv_vtl_poll_file {
>> + struct file *file;
>> + wait_queue_entry_t wait;
>> + wait_queue_head_t *wqh;
>> + poll_table pt;
>> + int cpu;
>> +};
>> +
>> +struct mshv_vtl {
>> + struct device *module_dev;
>> + u64 id;
>> +};
>> +
>> +union mshv_synic_overlay_page_msr {
>> + u64 as_uint64;
>> + struct {
>> + u64 enabled: 1;
>> + u64 reserved: 11;
>> + u64 pfn: 52;
>> + };
>
> Since this appear to be a Hyper-V synthetic MSR, add __packed?
Acked.
>
>> +};
>> +
>> +union hv_register_vsm_capabilities {
>> + u64 as_uint64;
>> + struct {
>> + u64 dr6_shared: 1;
>> + u64 mbec_vtl_mask: 16;
>> + u64 deny_lower_vtl_startup: 1;
>> + u64 supervisor_shadow_stack: 1;
>> + u64 hardware_hvpt_available: 1;
>> + u64 software_hvpt_available: 1;
>> + u64 hardware_hvpt_range_bits: 6;
>> + u64 intercept_page_available: 1;
>> + u64 return_action_available: 1;
>> + u64 reserved: 35;
>> + } __packed;
>> +};
>> +
>> +union hv_register_vsm_page_offsets {
>> + struct {
>> + u64 vtl_call_offset : 12;
>> + u64 vtl_return_offset : 12;
>> + u64 reserved_mbz : 40;
>> + };
>> + u64 as_uint64;
>> +} __packed;
>
> We've usually put the __packed on the struct definition. Consistency .... :-)
Acked. Will move __packed from union to struct.
>
> Don't these three register definitions belong somewhere in the
> hvhdk or hvgdk include files?
>
Moved them to hvgdk_mini.c
Will move hv_register_vsm_capabilities and hv_register_vsm_page_offsets
to hvgdk_mini.h and keep mshv_synic_overlay_page_msr here, with the
comment that Nuno pointed in his reply.
>> +
>> +struct mshv_vtl_per_cpu {
>> + struct mshv_vtl_run *run;
>> + struct page *reg_page;
>> +};
>> +
>> +static struct mutex mshv_vtl_poll_file_lock;
>> +static union hv_register_vsm_page_offsets mshv_vsm_page_offsets;
>> +static union hv_register_vsm_capabilities mshv_vsm_capabilities;
>> +
>> +static DEFINE_PER_CPU(struct mshv_vtl_poll_file, mshv_vtl_poll_file);
>> +static DEFINE_PER_CPU(unsigned long long, num_vtl0_transitions);
>> +static DEFINE_PER_CPU(struct mshv_vtl_per_cpu, mshv_vtl_per_cpu);
>> +
>> +static const struct file_operations mshv_vtl_fops;
>> +
>> +static long
>> +mshv_ioctl_create_vtl(void __user *user_arg, struct device *module_dev)
>> +{
>> + struct mshv_vtl *vtl;
>> + struct file *file;
>> + int fd;
>> +
>> + vtl = kzalloc(sizeof(*vtl), GFP_KERNEL);
>> + if (!vtl)
>> + return -ENOMEM;
>> +
>> + fd = get_unused_fd_flags(O_CLOEXEC);
>> + if (fd < 0) {
>> + kfree(vtl);
>> + return fd;
>> + }
>> + file = anon_inode_getfile("mshv_vtl", &mshv_vtl_fops,
>> + vtl, O_RDWR);
>> + if (IS_ERR(file)) {
>> + kfree(vtl);
>> + return PTR_ERR(file);
>> + }
>> + vtl->module_dev = module_dev;
>> + fd_install(fd, file);
>> +
>> + return fd;
>> +}
>> +
>> +static long
>> +mshv_ioctl_check_extension(void __user *user_arg)
>> +{
>> + u32 arg;
>> +
>> + if (copy_from_user(&arg, user_arg, sizeof(arg)))
>> + return -EFAULT;
>> +
>> + switch (arg) {
>> + case MSHV_CAP_CORE_API_STABLE:
>> + return 0;
>> + case MSHV_CAP_REGISTER_PAGE:
>> + return mshv_has_reg_page;
>> + case MSHV_CAP_VTL_RETURN_ACTION:
>> + return mshv_vsm_capabilities.return_action_available;
>> + case MSHV_CAP_DR6_SHARED:
>> + return mshv_vsm_capabilities.dr6_shared;
>> + }
>> +
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static long
>> +mshv_dev_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>> +{
>> + struct miscdevice *misc = filp->private_data;
>> +
>> + switch (ioctl) {
>> + case MSHV_CHECK_EXTENSION:
>> + return mshv_ioctl_check_extension((void __user *)arg);
>> + case MSHV_CREATE_VTL:
>> + return mshv_ioctl_create_vtl((void __user *)arg, misc->this_device);
>> + }
>> +
>> + return -ENOTTY;
>> +}
>> +
>> +static const struct file_operations mshv_dev_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = mshv_dev_ioctl,
>> + .llseek = noop_llseek,
>> +};
>> +
>> +static struct miscdevice mshv_dev = {
>> + .minor = MISC_DYNAMIC_MINOR,
>> + .name = "mshv",
>> + .fops = &mshv_dev_fops,
>> + .mode = 0600,
>> +};
>> +
>> +static struct mshv_vtl_run *mshv_vtl_this_run(void)
>> +{
>> + return *this_cpu_ptr(&mshv_vtl_per_cpu.run);
>> +}
>> +
>> +static struct mshv_vtl_run *mshv_vtl_cpu_run(int cpu)
>> +{
>> + return *per_cpu_ptr(&mshv_vtl_per_cpu.run, cpu);
>> +}
>> +
>> +static struct page *mshv_vtl_cpu_reg_page(int cpu)
>> +{
>> + return *per_cpu_ptr(&mshv_vtl_per_cpu.reg_page, cpu);
>> +}
>> +
>> +static void mshv_vtl_configure_reg_page(struct mshv_vtl_per_cpu *per_cpu)
>> +{
>> + struct hv_register_assoc reg_assoc = {};
>> + union mshv_synic_overlay_page_msr overlay = {};
>> + struct page *reg_page;
>> + union hv_input_vtl vtl = { .as_uint8 = 0 };
>
> This is the first of several places in this module where you need an input_vtl
> value of 0. Rather than declare and initialize a local variable for each use
> case, I'd suggest declaring a global static at the top of the module, and reuse
> that global whenever needed.
>
Acked.
>> +
>> + reg_page = alloc_page(GFP_KERNEL | __GFP_ZERO | __GFP_RETRY_MAYFAIL);
>> + if (!reg_page) {
>> + WARN(1, "failed to allocate register page\n");
>> + return;
>> + }
>> +
>> + overlay.enabled = 1;
>> + overlay.pfn = page_to_phys(reg_page) >> HV_HYP_PAGE_SHIFT;
>
> Could use page_to_hvpfn() here.
Acked.
>
>> + reg_assoc.name = HV_X64_REGISTER_REG_PAGE;
>> + reg_assoc.value.reg64 = overlay.as_uint64;
>> +
>> + if (hv_call_set_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> + 1, vtl, ®_assoc)) {
>> + WARN(1, "failed to setup register page\n");
>> + __free_page(reg_page);
>> + return;
>> + }
>> +
>> + per_cpu->reg_page = reg_page;
>> + mshv_has_reg_page = true;
>> +}
>> +
>> +static void mshv_vtl_synic_enable_regs(unsigned int cpu)
>> +{
>> + union hv_synic_sint sint;
>> +
>> + sint.as_uint64 = 0;
>> + sint.vector = HYPERVISOR_CALLBACK_VECTOR;
>> + sint.masked = false;
>> + sint.auto_eoi = hv_recommend_using_aeoi();
>> +
>> + /* Enable intercepts */
>> + if (!mshv_vsm_capabilities.intercept_page_available)
>> + hv_set_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX,
>> + sint.as_uint64);
>> +
>> + /* VTL2 Host VSP SINT is (un)masked when the user mode requests that */
>> +}
>> +
>> +static int mshv_vtl_get_vsm_regs(void)
>> +{
>> + struct hv_register_assoc registers[2];
>> + union hv_input_vtl input_vtl;
>> + int ret, count = 2;
>> +
>> + input_vtl.as_uint8 = 0;
>
> One of the places I mentioned earlier where you could use global value
> instead of having to set up input_vtl here.
Will take care of all instances.
>
>> + registers[0].name = HV_REGISTER_VSM_CODE_PAGE_OFFSETS;
>> + registers[1].name = HV_REGISTER_VSM_CAPABILITIES;
>> +
>> + ret = hv_call_get_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> + count, input_vtl, registers);
>> + if (ret)
>> + return ret;
>> +
>> + mshv_vsm_page_offsets.as_uint64 = registers[0].value.reg64;
>> + mshv_vsm_capabilities.as_uint64 = registers[1].value.reg64;
>> +
>> + return ret;
>> +}
>> +
>> +static int mshv_vtl_configure_vsm_partition(struct device *dev)
>> +{
>> + union hv_register_vsm_partition_config config;
>> + struct hv_register_assoc reg_assoc;
>> + union hv_input_vtl input_vtl;
>> +
>> + config.as_uint64 = 0;
>> + config.default_vtl_protection_mask = HV_MAP_GPA_PERMISSIONS_MASK;
>> + config.enable_vtl_protection = 1;
>> + config.zero_memory_on_reset = 1;
>> + config.intercept_vp_startup = 1;
>> + config.intercept_cpuid_unimplemented = 1;
>> +
>> + if (mshv_vsm_capabilities.intercept_page_available) {
>> + dev_dbg(dev, "using intercept page\n");
>> + config.intercept_page = 1;
>> + }
>> +
>> + reg_assoc.name = HV_REGISTER_VSM_PARTITION_CONFIG;
>> + reg_assoc.value.reg64 = config.as_uint64;
>> + input_vtl.as_uint8 = 0;
>
> Another place to use the global input_vtl.
Acked.
>
>> +
>> + return hv_call_set_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> + 1, input_vtl, ®_assoc);
>> +}
>> +
>> +static void mshv_vtl_vmbus_isr(void)
>> +{
>> + struct hv_per_cpu_context *per_cpu;
>> + struct hv_message *msg;
>> + u32 message_type;
>> + union hv_synic_event_flags *event_flags;
>> + unsigned long word;
>> + int i, j;
>> + struct eventfd_ctx *eventfd;
>> +
>> + per_cpu = this_cpu_ptr(hv_context.cpu_context);
>> + if (smp_processor_id() == 0) {
>> + msg = (struct hv_message *)per_cpu->synic_message_page + VTL2_VMBUS_SINT_INDEX;
>> + message_type = READ_ONCE(msg->header.message_type);
>> + if (message_type != HVMSG_NONE)
>> + tasklet_schedule(&msg_dpc);
>> + }
>> +
>> + event_flags = (union hv_synic_event_flags *)per_cpu->synic_event_page +
>> + VTL2_VMBUS_SINT_INDEX;
>> + for (i = 0; i < HV_EVENT_FLAGS_LONG_COUNT; i++) {
>> + if (READ_ONCE(event_flags->flags[i])) {
>> + word = xchg(&event_flags->flags[i], 0);
>> + for_each_set_bit(j, &word, BITS_PER_LONG) {
>
> Is there a reason for the complexity in finding and resetting bits that are
> set in the sync_event_page? See the code in vmbus_chan_sched() that I
> think is doing the same thing, but with simpler code.
I am sorry, but I am not sure how this can be written similar to
vmbus_chan_sched(). We don't have eventfd signaling mechanism there.
Can you please share some more info/code snippet of what you were
suggesting?
>
>> + rcu_read_lock();
>> + eventfd = READ_ONCE(flag_eventfds[i * BITS_PER_LONG + j]);
>> + if (eventfd)
>> + eventfd_signal(eventfd);
>> + rcu_read_unlock();
>> + }
>> + }
>> + }
>> +
>> + vmbus_isr();
>> +}
>> +
>> +static int mshv_vtl_alloc_context(unsigned int cpu)
>> +{
>> + struct mshv_vtl_per_cpu *per_cpu = this_cpu_ptr(&mshv_vtl_per_cpu);
>> + struct page *run_page;
>> +
>> + run_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>
> Nit: Using __get_free_page() here would avoid having to deal with struct page,
> and having to translate from struct page to a virtual address.
Acked.
>
>> + if (!run_page)
>> + return -ENOMEM;
>> +
>> + per_cpu->run = page_address(run_page);
>> + if (mshv_vsm_capabilities.intercept_page_available)
>> + mshv_vtl_configure_reg_page(per_cpu);
>> +
>> + mshv_vtl_synic_enable_regs(cpu);
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_cpuhp_online;
>> +
>> +static int hv_vtl_setup_synic(void)
>> +{
>> + int ret;
>> +
>> + /* Use our isr to first filter out packets destined for userspace */
>> + hv_setup_vmbus_handler(mshv_vtl_vmbus_isr);
>
> Will the normal vmbus_isr() already be setup as the vmbus_handler when this
> is called? If so, then this will overwrite vmbus_isr() with mshv_vtl_vmbus_isr().
In my experiments, yes.
>
>> +
>> + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vtl:online",
>> + mshv_vtl_alloc_context, NULL);
>> + if (ret < 0) {
>> + hv_remove_vmbus_handler();
>
> But the error case won't restore vmbus_isr(). It will set vmbus_handler to NULL.
> I'm not sure if that's OK or not. Maybe it doesn't really matter.
In this fail case, mshv_vtl_init would abort, and things would not move
forward in OpenHCL. However, for completeness sake, I can replace
s/ hv_remove_vmbus_handler() / hv_setup_vmbus_handler(vmbus_isr)
>
>> + return ret;
>> + }
>> +
>> + mshv_vtl_cpuhp_online = ret;
>> +
>> + return 0;
>> +}
>> +
>> +static void hv_vtl_remove_synic(void)
>> +{
>> + hv_remove_vmbus_handler();
>> + cpuhp_remove_state(mshv_vtl_cpuhp_online);
>
> Unless there's a reason otherwise, for symmetry these two steps
> should be done in the opposite order. The "remove" operation should
> typically do things in the reverse order of the "setup" operation.
>
> And again, note that this won't restore vmbus_handler if
> vmbus_isr() was overwritten in hv_vtl_setup_synic().
>
Acked. Will reorder this and replace remove with setup again.
>> +}
>> +
>> +static int vtl_get_vp_registers(u16 count,
>> + struct hv_register_assoc *registers)
>> +{
>> + union hv_input_vtl input_vtl;
>> +
>> + input_vtl.as_uint8 = 0;
>> + input_vtl.use_target_vtl = 1;
>
> You can also setup a static global for this case.
Acked.
>
>> +
>> + return hv_call_get_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> + count, input_vtl, registers);
>> +}
>> +
>> +static int vtl_set_vp_registers(u16 count,
>> + struct hv_register_assoc *registers)
>> +{
>> + union hv_input_vtl input_vtl;
>> +
>> + input_vtl.as_uint8 = 0;
>> + input_vtl.use_target_vtl = 1;
>
> And reuse the global here.
Acked.
>
>> +
>> + return hv_call_set_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> + count, input_vtl, registers);
>> +}
>> +
>> +static int mshv_vtl_ioctl_add_vtl0_mem(struct mshv_vtl *vtl, void __user *arg)
>> +{
>> + struct mshv_vtl_ram_disposition vtl0_mem;
>> + struct dev_pagemap *pgmap;
>> + void *addr;
>> +
>> + if (copy_from_user(&vtl0_mem, arg, sizeof(vtl0_mem)))
>> + return -EFAULT;
>> +
>> + if (vtl0_mem.last_pfn <= vtl0_mem.start_pfn) {
>> + dev_err(vtl->module_dev, "range start pfn (%llx) > end pfn (%llx)\n",
>> + vtl0_mem.start_pfn, vtl0_mem.last_pfn);
>> + return -EFAULT;
>> + }
>> +
>> + pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
>> + if (!pgmap)
>> + return -ENOMEM;
>> +
>> + pgmap->ranges[0].start = PFN_PHYS(vtl0_mem.start_pfn);
>> + pgmap->ranges[0].end = PFN_PHYS(vtl0_mem.last_pfn) - 1;
>
> Perhaps this should be
>
> pgmap->ranges[0].end = PFN_PHYS(vtl0_mem.last_pfn + 1) - 1
>
> otherwise the last page won't be included in the range. Or is excluding the
> last page intentional?
Excluding the last page is intentional. Hence there is a check for this
as well:
if (vtl0_mem.last_pfn <= vtl0_mem.start_pfn) {
>
>> + pgmap->nr_range = 1;
>> + pgmap->type = MEMORY_DEVICE_GENERIC;
>> +
>> + /*
>> + * Determine the highest page order that can be used for the given memory range.
>> + * This works best when the range is aligned; i.e. both the start and the length.
>> + */
>> + pgmap->vmemmap_shift = count_trailing_zeros(vtl0_mem.start_pfn | vtl0_mem.last_pfn);
>> + dev_dbg(vtl->module_dev,
>> + "Add VTL0 memory: start: 0x%llx, end_pfn: 0x%llx, page order: %lu\n",
>> + vtl0_mem.start_pfn, vtl0_mem.last_pfn, pgmap->vmemmap_shift);
>> +
>> + addr = devm_memremap_pages(mem_dev, pgmap);
>> + if (IS_ERR(addr)) {
>> + dev_err(vtl->module_dev, "devm_memremap_pages error: %ld\n", PTR_ERR(addr));
>> + kfree(pgmap);
>> + return -EFAULT;
>> + }
>> +
>> + /* Don't free pgmap, since it has to stick around until the memory
>> + * is unmapped, which will never happen as there is no scenario
>> + * where VTL0 can be released/shutdown without bringing down VTL2.
>> + */
>> + return 0;
>> +}
>> +
>> +static void mshv_vtl_cancel(int cpu)
>> +{
>> + int here = get_cpu();
>> +
>> + if (here != cpu) {
>> + if (!xchg_relaxed(&mshv_vtl_cpu_run(cpu)->cancel, 1))
>> + smp_send_reschedule(cpu);
>> + } else {
>> + WRITE_ONCE(mshv_vtl_this_run()->cancel, 1);
>> + }
>> + put_cpu();
>> +}
>> +
>> +static int mshv_vtl_poll_file_wake(wait_queue_entry_t *wait, unsigned int mode, int sync, void *key)
>> +{
>> + struct mshv_vtl_poll_file *poll_file = container_of(wait, struct mshv_vtl_poll_file, wait);
>> +
>> + mshv_vtl_cancel(poll_file->cpu);
>> +
>> + return 0;
>> +}
>> +
>> +static void mshv_vtl_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh, poll_table *pt)
>> +{
>> + struct mshv_vtl_poll_file *poll_file = container_of(pt, struct mshv_vtl_poll_file, pt);
>> +
>> + WARN_ON(poll_file->wqh);
>> + poll_file->wqh = wqh;
>> + add_wait_queue(wqh, &poll_file->wait);
>> +}
>> +
>> +static int mshv_vtl_ioctl_set_poll_file(struct mshv_vtl_set_poll_file __user *user_input)
>> +{
>> + struct file *file, *old_file;
>> + struct mshv_vtl_poll_file *poll_file;
>> + struct mshv_vtl_set_poll_file input;
>> +
>> + if (copy_from_user(&input, user_input, sizeof(input)))
>> + return -EFAULT;
>
> Seems like input.cpu should be checked to make sure it's less than nr_cpu_ids.
> cpu_online() uses the value to index into a bit array without doing any checks
> unless CONFIG_DEBUG_PER_CPU_MAPS is set.
Thanks, will add that.
>
>> +
>> + if (!cpu_online(input.cpu))
>> + return -EINVAL;
>
> Having tested that the target CPU is online, does anything ensure that the
> CPU stays online during the completion of this function? Usually the
> cpus_read_lock() needs to be held to ensure that an online CPU stays
> online for the duration of an operation.
Added cpus_read_lock() block around per_cpu_ptr operation. In general,
CPUs are never hotplugged in kernel from our Usecase POV. I have omitted
adding these locks at other places for now. Please let me know your
thoughts on this, in case you feel we need to have it.
>
>> +
>> + file = NULL;
>> + file = fget(input.fd);
>> + if (!file)
>> + return -EBADFD;
>> +
>> + poll_file = per_cpu_ptr(&mshv_vtl_poll_file, READ_ONCE(input.cpu));
>> + if (!poll_file)
>> + return -EINVAL;
>
> Testing poll_file here confused me a bit. per_cpu_ptr() just does some pointer
> arithmetic and doesn't return NULL from what I can tell.
per_cpu_ptr may point to wrong addresses if input.cpu is not correct or
greater than NR_CPUS. In any case, it should not be NULL. So I will
remove it. Thanks for noticing this.
>
>> +
>> + mutex_lock(&mshv_vtl_poll_file_lock);
>> +
>> + if (poll_file->wqh)
>> + remove_wait_queue(poll_file->wqh, &poll_file->wait);
>> + poll_file->wqh = NULL;
>> +
>> + old_file = poll_file->file;
>> + poll_file->file = file;
>> + poll_file->cpu = input.cpu;
>> +
>> + if (file) {
>> + init_waitqueue_func_entry(&poll_file->wait, mshv_vtl_poll_file_wake);
>> + init_poll_funcptr(&poll_file->pt, mshv_vtl_ptable_queue_proc);
>> + vfs_poll(file, &poll_file->pt);
>> + }
>> +
>> + mutex_unlock(&mshv_vtl_poll_file_lock);
>> +
>> + if (old_file)
>> + fput(old_file);
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_set_reg(struct hv_register_assoc *regs)
>> +{
>> + u64 reg64;
>> + enum hv_register_name gpr_name;
>> +
>> + gpr_name = regs->name;
>> + reg64 = regs->value.reg64;
>> +
>> + switch (gpr_name) {
>> + case HV_X64_REGISTER_DR0:
>> + native_set_debugreg(0, reg64);
>> + break;
>> + case HV_X64_REGISTER_DR1:
>> + native_set_debugreg(1, reg64);
>> + break;
>> + case HV_X64_REGISTER_DR2:
>> + native_set_debugreg(2, reg64);
>> + break;
>> + case HV_X64_REGISTER_DR3:
>> + native_set_debugreg(3, reg64);
>> + break;
>> + case HV_X64_REGISTER_DR6:
>> + if (!mshv_vsm_capabilities.dr6_shared)
>> + goto hypercall;
>> + native_set_debugreg(6, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_CAP:
>> + wrmsrl(MSR_MTRRcap, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_DEF_TYPE:
>> + wrmsrl(MSR_MTRRdefType, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE0:
>> + wrmsrl(MTRRphysBase_MSR(0), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE1:
>> + wrmsrl(MTRRphysBase_MSR(1), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE2:
>> + wrmsrl(MTRRphysBase_MSR(2), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE3:
>> + wrmsrl(MTRRphysBase_MSR(3), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE4:
>> + wrmsrl(MTRRphysBase_MSR(4), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE5:
>> + wrmsrl(MTRRphysBase_MSR(5), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE6:
>> + wrmsrl(MTRRphysBase_MSR(6), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE7:
>> + wrmsrl(MTRRphysBase_MSR(7), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE8:
>> + wrmsrl(MTRRphysBase_MSR(8), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE9:
>> + wrmsrl(MTRRphysBase_MSR(9), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEA:
>> + wrmsrl(MTRRphysBase_MSR(0xa), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEB:
>> + wrmsrl(MTRRphysBase_MSR(0xb), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEC:
>> + wrmsrl(MTRRphysBase_MSR(0xc), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASED:
>> + wrmsrl(MTRRphysBase_MSR(0xd), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEE:
>> + wrmsrl(MTRRphysBase_MSR(0xe), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEF:
>> + wrmsrl(MTRRphysBase_MSR(0xf), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK0:
>> + wrmsrl(MTRRphysMask_MSR(0), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK1:
>> + wrmsrl(MTRRphysMask_MSR(1), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK2:
>> + wrmsrl(MTRRphysMask_MSR(2), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK3:
>> + wrmsrl(MTRRphysMask_MSR(3), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK4:
>> + wrmsrl(MTRRphysMask_MSR(4), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK5:
>> + wrmsrl(MTRRphysMask_MSR(5), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK6:
>> + wrmsrl(MTRRphysMask_MSR(6), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK7:
>> + wrmsrl(MTRRphysMask_MSR(7), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK8:
>> + wrmsrl(MTRRphysMask_MSR(8), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK9:
>> + wrmsrl(MTRRphysMask_MSR(9), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKA:
>> + wrmsrl(MTRRphysMask_MSR(0xa), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKB:
>> + wrmsrl(MTRRphysMask_MSR(0xb), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKC:
>> + wrmsrl(MTRRphysMask_MSR(0xc), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKD:
>> + wrmsrl(MTRRphysMask_MSR(0xd), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKE:
>> + wrmsrl(MTRRphysMask_MSR(0xe), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKF:
>> + wrmsrl(MTRRphysMask_MSR(0xf), reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX64K00000:
>> + wrmsrl(MSR_MTRRfix64K_00000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX16K80000:
>> + wrmsrl(MSR_MTRRfix16K_80000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX16KA0000:
>> + wrmsrl(MSR_MTRRfix16K_A0000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KC0000:
>> + wrmsrl(MSR_MTRRfix4K_C0000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KC8000:
>> + wrmsrl(MSR_MTRRfix4K_C8000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KD0000:
>> + wrmsrl(MSR_MTRRfix4K_D0000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KD8000:
>> + wrmsrl(MSR_MTRRfix4K_D8000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KE0000:
>> + wrmsrl(MSR_MTRRfix4K_E0000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KE8000:
>> + wrmsrl(MSR_MTRRfix4K_E8000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KF0000:
>> + wrmsrl(MSR_MTRRfix4K_F0000, reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KF8000:
>> + wrmsrl(MSR_MTRRfix4K_F8000, reg64);
>> + break;
>> +
>> + default:
>> + goto hypercall;
>> + }
>> +
>> + return 0;
>> +
>> +hypercall:
>> + return 1;
>> +}
>
> To me, this function and the follow-on mshv_vtl_get_reg() are just
> begging to be implemented using a static table that maps from
> HV_X64_REGISTER_MSR* to MSR* (at least for the ~45 entries that
> are not debug registers). Each of the functions could then search
> through the table to find the HV_X64_REGISTER* and get the
> matching x86 MSR value, followed by a single invocation of wrmsr()
> or rdmsrl(). The generated code for mshv_vtl_set_reg() is about
> 1550 bytes, and a table search would be a lot smaller. The size
> of mshv_vtl_get_reg() would be similarly reduced using the same
> table. You'd probably end up with at least 100 fewer source code
> lines as well.
>
:) makes sense, will change it.
>> +
>> +static int mshv_vtl_get_reg(struct hv_register_assoc *regs)
>> +{
>> + u64 *reg64;
>> + enum hv_register_name gpr_name;
>> +
>> + gpr_name = regs->name;
>> + reg64 = (u64 *)®s->value.reg64;
>> +
>> + switch (gpr_name) {
>> + case HV_X64_REGISTER_DR0:
>> + *reg64 = native_get_debugreg(0);
>> + break;
>> + case HV_X64_REGISTER_DR1:
>> + *reg64 = native_get_debugreg(1);
>> + break;
>> + case HV_X64_REGISTER_DR2:
>> + *reg64 = native_get_debugreg(2);
>> + break;
>> + case HV_X64_REGISTER_DR3:
>> + *reg64 = native_get_debugreg(3);
>> + break;
>> + case HV_X64_REGISTER_DR6:
>> + if (!mshv_vsm_capabilities.dr6_shared)
>> + goto hypercall;
>> + *reg64 = native_get_debugreg(6);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_CAP:
>> + rdmsrl(MSR_MTRRcap, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_DEF_TYPE:
>> + rdmsrl(MSR_MTRRdefType, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE0:
>> + rdmsrl(MTRRphysBase_MSR(0), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE1:
>> + rdmsrl(MTRRphysBase_MSR(1), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE2:
>> + rdmsrl(MTRRphysBase_MSR(2), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE3:
>> + rdmsrl(MTRRphysBase_MSR(3), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE4:
>> + rdmsrl(MTRRphysBase_MSR(4), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE5:
>> + rdmsrl(MTRRphysBase_MSR(5), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE6:
>> + rdmsrl(MTRRphysBase_MSR(6), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE7:
>> + rdmsrl(MTRRphysBase_MSR(7), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE8:
>> + rdmsrl(MTRRphysBase_MSR(8), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASE9:
>> + rdmsrl(MTRRphysBase_MSR(9), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEA:
>> + rdmsrl(MTRRphysBase_MSR(0xa), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEB:
>> + rdmsrl(MTRRphysBase_MSR(0xb), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEC:
>> + rdmsrl(MTRRphysBase_MSR(0xc), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASED:
>> + rdmsrl(MTRRphysBase_MSR(0xd), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEE:
>> + rdmsrl(MTRRphysBase_MSR(0xe), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_BASEF:
>> + rdmsrl(MTRRphysBase_MSR(0xf), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK0:
>> + rdmsrl(MTRRphysMask_MSR(0), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK1:
>> + rdmsrl(MTRRphysMask_MSR(1), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK2:
>> + rdmsrl(MTRRphysMask_MSR(2), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK3:
>> + rdmsrl(MTRRphysMask_MSR(3), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK4:
>> + rdmsrl(MTRRphysMask_MSR(4), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK5:
>> + rdmsrl(MTRRphysMask_MSR(5), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK6:
>> + rdmsrl(MTRRphysMask_MSR(6), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK7:
>> + rdmsrl(MTRRphysMask_MSR(7), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK8:
>> + rdmsrl(MTRRphysMask_MSR(8), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASK9:
>> + rdmsrl(MTRRphysMask_MSR(9), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKA:
>> + rdmsrl(MTRRphysMask_MSR(0xa), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKB:
>> + rdmsrl(MTRRphysMask_MSR(0xb), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKC:
>> + rdmsrl(MTRRphysMask_MSR(0xc), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKD:
>> + rdmsrl(MTRRphysMask_MSR(0xd), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKE:
>> + rdmsrl(MTRRphysMask_MSR(0xe), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_PHYS_MASKF:
>> + rdmsrl(MTRRphysMask_MSR(0xf), *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX64K00000:
>> + rdmsrl(MSR_MTRRfix64K_00000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX16K80000:
>> + rdmsrl(MSR_MTRRfix16K_80000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX16KA0000:
>> + rdmsrl(MSR_MTRRfix16K_A0000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KC0000:
>> + rdmsrl(MSR_MTRRfix4K_C0000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KC8000:
>> + rdmsrl(MSR_MTRRfix4K_C8000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KD0000:
>> + rdmsrl(MSR_MTRRfix4K_D0000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KD8000:
>> + rdmsrl(MSR_MTRRfix4K_D8000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KE0000:
>> + rdmsrl(MSR_MTRRfix4K_E0000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KE8000:
>> + rdmsrl(MSR_MTRRfix4K_E8000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KF0000:
>> + rdmsrl(MSR_MTRRfix4K_F0000, *reg64);
>> + break;
>> + case HV_X64_REGISTER_MSR_MTRR_FIX4KF8000:
>> + rdmsrl(MSR_MTRRfix4K_F8000, *reg64);
>> + break;
>> +
>> + default:
>> + goto hypercall;
>> + }
>> +
>> + return 0;
>> +
>> +hypercall:
>> + return 1;
>> +}
>> +
>> +static void mshv_vtl_return(struct mshv_vtl_cpu_context *vtl0)
>> +{
>> + struct hv_vp_assist_page *hvp;
>> + u64 hypercall_addr;
>> +
>> + register u64 r8 asm("r8");
>> + register u64 r9 asm("r9");
>> + register u64 r10 asm("r10");
>> + register u64 r11 asm("r11");
>> + register u64 r12 asm("r12");
>> + register u64 r13 asm("r13");
>> + register u64 r14 asm("r14");
>> + register u64 r15 asm("r15");
>> +
>> + hvp = hv_vp_assist_page[smp_processor_id()];
>> +
>> + /*
>> + * Process signal event direct set in the run page, if any.
>> + */
>> + if (mshv_vsm_capabilities.return_action_available) {
>> + u32 offset = READ_ONCE(mshv_vtl_this_run()->vtl_ret_action_size);
>> +
>> + WRITE_ONCE(mshv_vtl_this_run()->vtl_ret_action_size, 0);
>> +
>> + /*
>> + * Hypervisor will take care of clearing out the actions
>> + * set in the assist page.
>> + */
>> + memcpy(hvp->vtl_ret_actions,
>> + mshv_vtl_this_run()->vtl_ret_actions,
>> + min_t(u32, offset, sizeof(hvp->vtl_ret_actions)));
>> + }
>> +
>> + hvp->vtl_ret_x64rax = vtl0->rax;
>> + hvp->vtl_ret_x64rcx = vtl0->rcx;
>> +
>> + hypercall_addr = (u64)((u8 *)hv_hypercall_pg + mshv_vsm_page_offsets.vtl_return_offset);
>> +
>> + kernel_fpu_begin_mask(0);
>> + fxrstor(&vtl0->fx_state);
>> + native_write_cr2(vtl0->cr2);
>> + r8 = vtl0->r8;
>> + r9 = vtl0->r9;
>> + r10 = vtl0->r10;
>> + r11 = vtl0->r11;
>> + r12 = vtl0->r12;
>> + r13 = vtl0->r13;
>> + r14 = vtl0->r14;
>> + r15 = vtl0->r15;
>> +
>> + asm __volatile__ ( \
>> + /* Save rbp pointer to the lower VTL, keep the stack 16-byte aligned */
>> + "pushq %%rbp\n"
>> + "pushq %%rcx\n"
>> + /* Restore the lower VTL's rbp */
>> + "movq (%%rcx), %%rbp\n"
>> + /* Load return kind into rcx (HV_VTL_RETURN_INPUT_NORMAL_RETURN == 0)
>> */
>> + "xorl %%ecx, %%ecx\n"
>> + /* Transition to the lower VTL */
>> + CALL_NOSPEC
>> + /* Save VTL0's rax and rcx temporarily on 16-byte aligned stack */
>> + "pushq %%rax\n"
>> + "pushq %%rcx\n"
>> + /* Restore pointer to lower VTL rbp */
>> + "movq 16(%%rsp), %%rax\n"
>> + /* Save the lower VTL's rbp */
>> + "movq %%rbp, (%%rax)\n"
>> + /* Restore saved registers */
>> + "movq 8(%%rsp), %%rax\n"
>> + "movq 24(%%rsp), %%rbp\n"
>> + "addq $32, %%rsp\n"
>> +
>> + : "=a"(vtl0->rax), "=c"(vtl0->rcx),
>> + "+d"(vtl0->rdx), "+b"(vtl0->rbx), "+S"(vtl0->rsi), "+D"(vtl0->rdi),
>> + "+r"(r8), "+r"(r9), "+r"(r10), "+r"(r11),
>> + "+r"(r12), "+r"(r13), "+r"(r14), "+r"(r15)
>> + : THUNK_TARGET(hypercall_addr), "c"(&vtl0->rbp)
>> + : "cc", "memory");
>> +
>> + vtl0->r8 = r8;
>> + vtl0->r9 = r9;
>> + vtl0->r10 = r10;
>> + vtl0->r11 = r11;
>> + vtl0->r12 = r12;
>> + vtl0->r13 = r13;
>> + vtl0->r14 = r14;
>> + vtl0->r15 = r15;
>> + vtl0->cr2 = native_read_cr2();
>> +
>> + fxsave(&vtl0->fx_state);
>> + kernel_fpu_end();
>> +}
>> +
>> +/*
>> + * Returning to a lower VTL treats the base pointer register
>> + * as a general purpose one. Without adding this, objtool produces
>> + * a warning.
>> + */
>> +STACK_FRAME_NON_STANDARD(mshv_vtl_return);
>> +
>> +static bool mshv_vtl_process_intercept(void)
>> +{
>> + struct hv_per_cpu_context *mshv_cpu;
>> + void *synic_message_page;
>> + struct hv_message *msg;
>> + u32 message_type;
>> +
>> + mshv_cpu = this_cpu_ptr(hv_context.cpu_context);
>> + synic_message_page = mshv_cpu->synic_message_page;
>> + if (unlikely(!synic_message_page))
>> + return true;
>> +
>> + msg = (struct hv_message *)synic_message_page + HV_SYNIC_INTERCEPTION_SINT_INDEX;
>> + message_type = READ_ONCE(msg->header.message_type);
>> + if (message_type == HVMSG_NONE)
>> + return true;
>> +
>> + memcpy(mshv_vtl_this_run()->exit_message, msg, sizeof(*msg));
>> + vmbus_signal_eom(msg, message_type);
>> +
>> + return false;
>> +}
>> +
>> +static int mshv_vtl_ioctl_return_to_lower_vtl(void)
>> +{
>> + preempt_disable();
>> + for (;;) {
>> + const unsigned long VTL0_WORK = _TIF_SIGPENDING | _TIF_NEED_RESCHED |
>> + _TIF_NOTIFY_RESUME | _TIF_NOTIFY_SIGNAL;
>> + unsigned long ti_work;
>> + u32 cancel;
>> + unsigned long irq_flags;
>> + struct hv_vp_assist_page *hvp;
>> + int ret;
>> +
>> + local_irq_save(irq_flags);
>> + ti_work = READ_ONCE(current_thread_info()->flags);
>> + cancel = READ_ONCE(mshv_vtl_this_run()->cancel);
>> + if (unlikely((ti_work & VTL0_WORK) || cancel)) {
>> + local_irq_restore(irq_flags);
>> + preempt_enable();
>> + if (cancel)
>> + ti_work |= _TIF_SIGPENDING;
>> + ret = mshv_do_pre_guest_mode_work(ti_work);
>> + if (ret)
>> + return ret;
>> + preempt_disable();
>> + continue;
>> + }
>> +
>> + mshv_vtl_return(&mshv_vtl_this_run()->cpu_context);
>> + local_irq_restore(irq_flags);
>> +
>> + hvp = hv_vp_assist_page[smp_processor_id()];
>> + this_cpu_inc(num_vtl0_transitions);
>> + switch (hvp->vtl_entry_reason) {
>> + case MSHV_ENTRY_REASON_INTERRUPT:
>> + if (!mshv_vsm_capabilities.intercept_page_available &&
>> + likely(!mshv_vtl_process_intercept()))
>> + goto done;
>> + break;
>> +
>> + case MSHV_ENTRY_REASON_INTERCEPT:
>> + WARN_ON(!mshv_vsm_capabilities.intercept_page_available);
>> + memcpy(mshv_vtl_this_run()->exit_message, hvp->intercept_message,
>> + sizeof(hvp->intercept_message));
>> + goto done;
>> +
>> + default:
>> + panic("unknown entry reason: %d", hvp->vtl_entry_reason);
>> + }
>> + }
>> +
>> +done:
>> + preempt_enable();
>> +
>> + return 0;
>> +}
>> +
>> +static long
>> +mshv_vtl_ioctl_get_regs(void __user *user_args)
>> +{
>> + struct mshv_vp_registers args;
>> + struct hv_register_assoc *registers;
>> + long ret;
>> +
>> + if (copy_from_user(&args, user_args, sizeof(args)))
>> + return -EFAULT;
>> +
>> + if (args.count == 0 || args.count > MSHV_VP_MAX_REGISTERS)
>> + return -EINVAL;
>> +
>> + registers = kmalloc_array(args.count,
>> + sizeof(*registers),
>> + GFP_KERNEL);
>> + if (!registers)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(registers, (void __user *)args.regs_ptr,
>> + sizeof(*registers) * args.count)) {
>> + ret = -EFAULT;
>> + goto free_return;
>> + }
>> +
>> + ret = mshv_vtl_get_reg(registers);
>> + if (!ret)
>> + goto copy_args; /* No need of hypercall */
>
> I'm puzzled by the functionality here. The input to this function may
> specify up to MSHV_VP_MAX_REGISTERS, which is 128. But if the
> first register in the input is handled by mshv_vtl_get_reg(), then
> no additional register values are retrieved. But the full set of
> register values are copied back to user space, all of which
> except the first are just whatever register values were originally
> copied in from user space.
Userspace in our case (OpenVMM) was always sending one register per
IOCTL, so this code always worked. However, to avoid confusion, I will
remove the consideration of args.count here and always process the first
register only. Will also add a note here. The feature could have been
extended to include a bunch of registers at once, but due to
complications in implementation of how many registers actually were set
successfully and to report back the failed registers to userspace, we
thought of keeping it simple as one register per IOCTL. This is also not
a performance sensitive path, so it should be fine.
>
>> + ret = vtl_get_vp_registers(args.count, registers);
>> + if (ret)
>> + goto free_return;
>> +
>> +copy_args:
>> + if (copy_to_user((void __user *)args.regs_ptr, registers,
>> + sizeof(*registers) * args.count))
>> + ret = -EFAULT;
>> +free_return:
>> + kfree(registers);
>> +
>> + return ret;
>> +}
>> +
>> +static long
>> +mshv_vtl_ioctl_set_regs(void __user *user_args)
>> +{
>> + struct mshv_vp_registers args;
>> + struct hv_register_assoc *registers;
>> + long ret;
>> +
>> + if (copy_from_user(&args, user_args, sizeof(args)))
>> + return -EFAULT;
>> +
>> + if (args.count == 0 || args.count > MSHV_VP_MAX_REGISTERS)
>> + return -EINVAL;
>> +
>> + registers = kmalloc_array(args.count,
>> + sizeof(*registers),
>> + GFP_KERNEL);
>> + if (!registers)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(registers, (void __user *)args.regs_ptr,
>> + sizeof(*registers) * args.count)) {
>> + ret = -EFAULT;
>> + goto free_return;
>> + }
>> +
>> + ret = mshv_vtl_set_reg(registers);
>> + if (!ret)
>> + goto free_return; /* No need of hypercall */
>
> Again, I'm puzzled by the functionality because only the first
> register value is set if mshv_vtl_set_reg() handles that register.
> The other specified register values are ignored.
Covered in my previous reply.
>
>> + ret = vtl_set_vp_registers(args.count, registers);
>> +
>> +free_return:
>> + kfree(registers);
>> +
>> + return ret;
>> +}
>> +
>> +static long
>> +mshv_vtl_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>> +{
>> + long ret;
>> + struct mshv_vtl *vtl = filp->private_data;
>> +
>> + switch (ioctl) {
>> + case MSHV_SET_POLL_FILE:
>> + ret = mshv_vtl_ioctl_set_poll_file((struct mshv_vtl_set_poll_file *)arg);
>> + break;
>> + case MSHV_GET_VP_REGISTERS:
>> + ret = mshv_vtl_ioctl_get_regs((void __user *)arg);
>> + break;
>> + case MSHV_SET_VP_REGISTERS:
>> + ret = mshv_vtl_ioctl_set_regs((void __user *)arg);
>> + break;
>> + case MSHV_RETURN_TO_LOWER_VTL:
>> + ret = mshv_vtl_ioctl_return_to_lower_vtl();
>> + break;
>> + case MSHV_ADD_VTL0_MEMORY:
>> + ret = mshv_vtl_ioctl_add_vtl0_mem(vtl, (void __user *)arg);
>> + break;
>> + default:
>> + dev_err(vtl->module_dev, "invalid vtl ioctl: %#x\n", ioctl);
>> + ret = -ENOTTY;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static vm_fault_t mshv_vtl_fault(struct vm_fault *vmf)
>> +{
>> + struct page *page;
>> + int cpu = vmf->pgoff & MSHV_PG_OFF_CPU_MASK;
>> + int real_off = vmf->pgoff >> MSHV_REAL_OFF_SHIFT;
>> +
>> + if (!cpu_online(cpu))
>> + return VM_FAULT_SIGBUS;
>
> Same question as earlier -- what ensures the CPU stays online
> after the above test?
It was based on assumptions, I will add cpu_read_lock/unlock.
>
>> +
>> + if (real_off == MSHV_RUN_PAGE_OFFSET) {
>> + page = virt_to_page(mshv_vtl_cpu_run(cpu));
>> + } else if (real_off == MSHV_REG_PAGE_OFFSET) {
>> + if (!mshv_has_reg_page)
>> + return VM_FAULT_SIGBUS;
>> + page = mshv_vtl_cpu_reg_page(cpu);
>> + } else {
>> + return VM_FAULT_NOPAGE;
>> + }
>> +
>> + get_page(page);
>> + vmf->page = page;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct vm_operations_struct mshv_vtl_vm_ops = {
>> + .fault = mshv_vtl_fault,
>> +};
>> +
>> +static int mshv_vtl_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + vma->vm_ops = &mshv_vtl_vm_ops;
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_release(struct inode *inode, struct file *filp)
>> +{
>> + struct mshv_vtl *vtl = filp->private_data;
>> +
>> + kfree(vtl);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations mshv_vtl_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = mshv_vtl_ioctl,
>> + .release = mshv_vtl_release,
>> + .mmap = mshv_vtl_mmap,
>> +};
>> +
>> +static void mshv_vtl_synic_mask_vmbus_sint(const u8 *mask)
>> +{
>> + union hv_synic_sint sint;
>> +
>> + sint.as_uint64 = 0;
>> + sint.vector = HYPERVISOR_CALLBACK_VECTOR;
>> + sint.masked = (*mask != 0);
>> + sint.auto_eoi = hv_recommend_using_aeoi();
>> +
>> + hv_set_msr(HV_MSR_SINT0 + VTL2_VMBUS_SINT_INDEX,
>> + sint.as_uint64);
>> +
>> + if (!sint.masked)
>> + pr_debug("%s: Unmasking VTL2 VMBUS SINT on VP %d\n", __func__, smp_processor_id());
>> + else
>> + pr_debug("%s: Masking VTL2 VMBUS SINT on VP %d\n", __func__, smp_processor_id());
>> +}
>> +
>> +static void mshv_vtl_read_remote(void *buffer)
>> +{
>> + struct hv_per_cpu_context *mshv_cpu = this_cpu_ptr(hv_context.cpu_context);
>> + struct hv_message *msg = (struct hv_message *)mshv_cpu->synic_message_page +
>> + VTL2_VMBUS_SINT_INDEX;
>> + u32 message_type = READ_ONCE(msg->header.message_type);
>> +
>> + WRITE_ONCE(has_message, false);
>> + if (message_type == HVMSG_NONE)
>> + return;
>> +
>> + memcpy(buffer, msg, sizeof(*msg));
>> + vmbus_signal_eom(msg, message_type);
>> +}
>> +
>> +static bool vtl_synic_mask_vmbus_sint_masked = true;
>> +
>> +static ssize_t mshv_vtl_sint_read(struct file *filp, char __user *arg, size_t size, loff_t
>> *offset)
>> +{
>> + struct hv_message msg = {};
>> + int ret;
>> +
>> + if (size < sizeof(msg))
>> + return -EINVAL;
>> +
>> + for (;;) {
>> + smp_call_function_single(VMBUS_CONNECT_CPU, mshv_vtl_read_remote, &msg, true);
>> + if (msg.header.message_type != HVMSG_NONE)
>> + break;
>> +
>> + if (READ_ONCE(vtl_synic_mask_vmbus_sint_masked))
>> + return 0; /* EOF */
>> +
>> + if (filp->f_flags & O_NONBLOCK)
>> + return -EAGAIN;
>> +
>> + ret = wait_event_interruptible(fd_wait_queue,
>> + READ_ONCE(has_message) ||
>> + READ_ONCE(vtl_synic_mask_vmbus_sint_masked));
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (copy_to_user(arg, &msg, sizeof(msg)))
>> + return -EFAULT;
>> +
>> + return sizeof(msg);
>> +}
>> +
>> +static __poll_t mshv_vtl_sint_poll(struct file *filp, poll_table *wait)
>> +{
>> + __poll_t mask = 0;
>> +
>> + poll_wait(filp, &fd_wait_queue, wait);
>> + if (READ_ONCE(has_message) || READ_ONCE(vtl_synic_mask_vmbus_sint_masked))
>> + mask |= EPOLLIN | EPOLLRDNORM;
>> +
>> + return mask;
>> +}
>> +
>> +static void mshv_vtl_sint_on_msg_dpc(unsigned long data)
>> +{
>> + WRITE_ONCE(has_message, true);
>> + wake_up_interruptible_poll(&fd_wait_queue, EPOLLIN);
>> +}
>> +
>> +static int mshv_vtl_sint_ioctl_post_message(struct mshv_vtl_sint_post_msg __user *arg)
>> +{
>> + struct mshv_vtl_sint_post_msg message;
>> + u8 payload[HV_MESSAGE_PAYLOAD_BYTE_COUNT];
>> +
>> + if (copy_from_user(&message, arg, sizeof(message)))
>> + return -EFAULT;
>> + if (message.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT)
>> + return -EINVAL;
>> + if (copy_from_user(payload, (void __user *)message.payload_ptr,
>> + message.payload_size))
>> + return -EFAULT;
>> +
>> + return hv_post_message((union hv_connection_id)message.connection_id,
>> + message.message_type, (void *)payload,
>> + message.payload_size);
>> +}
>> +
>> +static int mshv_vtl_sint_ioctl_signal_event(struct mshv_vtl_signal_event __user *arg)
>> +{
>> + u64 input;
>> + struct mshv_vtl_signal_event signal_event;
>> +
>> + if (copy_from_user(&signal_event, arg, sizeof(signal_event)))
>> + return -EFAULT;
>> +
>> + input = signal_event.connection_id | ((u64)signal_event.flag << 32);
>> +
>> + return hv_do_fast_hypercall8(HVCALL_SIGNAL_EVENT, input) & HV_HYPERCALL_RESULT_MASK;
>
> The return value is inconsistent here. It might be a negative errno if
> copy_from_user() fails, or it might be a Hyper-V hypercall result code. The
> caller is mshv_vtl_sint_ioctl(), which is called from vfs_ioctl(), which
> expects a return value of 0 on success, and a negative errno on error.
>
> You can use hv_result_to_errno() here to translate the hypercall result
> code to a negative errno.
>
Acked.
>> +}
>> +
>> +static int mshv_vtl_sint_ioctl_set_eventfd(struct mshv_vtl_set_eventfd __user *arg)
>> +{
>> + struct mshv_vtl_set_eventfd set_eventfd;
>> + struct eventfd_ctx *eventfd, *old_eventfd;
>> +
>> + if (copy_from_user(&set_eventfd, arg, sizeof(set_eventfd)))
>> + return -EFAULT;
>> + if (set_eventfd.flag >= HV_EVENT_FLAGS_COUNT)
>> + return -EINVAL;
>> +
>> + eventfd = NULL;
>> + if (set_eventfd.fd >= 0) {
>> + eventfd = eventfd_ctx_fdget(set_eventfd.fd);
>> + if (IS_ERR(eventfd))
>> + return PTR_ERR(eventfd);
>> + }
>> +
>> + mutex_lock(&flag_lock);
>> + old_eventfd = flag_eventfds[set_eventfd.flag];
>> + WRITE_ONCE(flag_eventfds[set_eventfd.flag], eventfd);
>> + mutex_unlock(&flag_lock);
>
> The above looks a little weird. Generally, if you are using READ_ONCE()
> or WRITE_ONCE() for a field, you should use them everywhere the field
> is referenced. I see that there is a READ_ONCE() in mshv_vtl_vmbus_isr().
> For robustness, I'd suggest using READ_ONCE() above to read the old value.
>
I added these to resolve some issues with one of the static analyser
tools. Initially they were not there. Will add READ_ONCE now.
>> +
>> + if (old_eventfd) {
>> + synchronize_rcu();
>> + eventfd_ctx_put(old_eventfd);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_sint_ioctl_pause_message_stream(struct mshv_sint_mask __user *arg)
>> +{
>> + static DEFINE_MUTEX(vtl2_vmbus_sint_mask_mutex);
>> + struct mshv_sint_mask mask;
>> +
>> + if (copy_from_user(&mask, arg, sizeof(mask)))
>> + return -EFAULT;
>> + mutex_lock(&vtl2_vmbus_sint_mask_mutex);
>> + on_each_cpu((smp_call_func_t)mshv_vtl_synic_mask_vmbus_sint, &mask.mask, 1);
>> + WRITE_ONCE(vtl_synic_mask_vmbus_sint_masked, mask.mask != 0);
>> + mutex_unlock(&vtl2_vmbus_sint_mask_mutex);
>> + if (mask.mask)
>> + wake_up_interruptible_poll(&fd_wait_queue, EPOLLIN);
>> +
>> + return 0;
>> +}
>> +
>> +static long mshv_vtl_sint_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>> +{
>> + switch (cmd) {
>> + case MSHV_SINT_POST_MESSAGE:
>> + return mshv_vtl_sint_ioctl_post_message((struct mshv_vtl_sint_post_msg *)arg);
>> + case MSHV_SINT_SIGNAL_EVENT:
>> + return mshv_vtl_sint_ioctl_signal_event((struct mshv_vtl_signal_event *)arg);
>> + case MSHV_SINT_SET_EVENTFD:
>> + return mshv_vtl_sint_ioctl_set_eventfd((struct mshv_vtl_set_eventfd *)arg);
>> + case MSHV_SINT_PAUSE_MESSAGE_STREAM:
>> + return mshv_vtl_sint_ioctl_pause_message_stream((struct mshv_sint_mask *)arg);
>> + default:
>> + return -ENOIOCTLCMD;
>> + }
>> +}
>> +
>> +static const struct file_operations mshv_vtl_sint_ops = {
>> + .owner = THIS_MODULE,
>> + .read = mshv_vtl_sint_read,
>> + .poll = mshv_vtl_sint_poll,
>> + .unlocked_ioctl = mshv_vtl_sint_ioctl,
>> +};
>> +
>> +static struct miscdevice mshv_vtl_sint_dev = {
>> + .name = "mshv_sint",
>> + .fops = &mshv_vtl_sint_ops,
>> + .mode = 0600,
>> + .minor = MISC_DYNAMIC_MINOR,
>> +};
>> +
>> +static int mshv_vtl_hvcall_open(struct inode *node, struct file *f)
>> +{
>> + struct miscdevice *dev = f->private_data;
>> + struct mshv_vtl_hvcall_fd *fd;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + fd = vzalloc(sizeof(*fd));
>> + if (!fd)
>> + return -ENOMEM;
>> + fd->dev = dev;
>> + f->private_data = fd;
>> + mutex_init(&fd->init_mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_hvcall_release(struct inode *node, struct file *f)
>> +{
>> + struct mshv_vtl_hvcall_fd *fd;
>> +
>> + fd = f->private_data;
>> + if (fd) {
>> + vfree(fd);
>> + f->private_data = NULL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mshv_vtl_hvcall_setup(struct mshv_vtl_hvcall_fd *fd,
>> + struct mshv_vtl_hvcall_setup __user *hvcall_setup_user)
>> +{
>> + int ret = 0;
>> + struct mshv_vtl_hvcall_setup hvcall_setup;
>> +
>> + mutex_lock(&fd->init_mutex);
>> +
>> + if (fd->allow_map_initialized) {
>> + dev_err(fd->dev->this_device,
>> + "Hypercall allow map has already been set, pid %d\n",
>> + current->pid);
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>> +
>> + if (copy_from_user(&hvcall_setup, hvcall_setup_user,
>> + sizeof(struct mshv_vtl_hvcall_setup))) {
>> + ret = -EFAULT;
>> + goto exit;
>> + }
>> + if (hvcall_setup.bitmap_size > ARRAY_SIZE(fd->allow_bitmap)) {
>
> This implies that "bitmap_size" is in units of u64.
Covered in earlier reply.
>
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>> + if (copy_from_user(&fd->allow_bitmap,
>> + (void __user *)hvcall_setup.allow_bitmap_ptr,
>> + hvcall_setup.bitmap_size)) {
>
> But this implies that "bitmap_size" is in units of bytes. So something is wrong.
Ditto.
>
>> + ret = -EFAULT;
>> + goto exit;
>> + }
>> +
>> + dev_info(fd->dev->this_device, "Hypercall allow map has been set, pid %d\n",
>> + current->pid);
>> + fd->allow_map_initialized = true;
>> +exit:
>> + mutex_unlock(&fd->init_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static bool mshv_vtl_hvcall_is_allowed(struct mshv_vtl_hvcall_fd *fd, u16 call_code)
>> +{
>> + u8 bits_per_item = 8 * sizeof(fd->allow_bitmap[0]);
>> + u16 item_index = call_code / bits_per_item;
>> + u64 mask = 1ULL << (call_code % bits_per_item);
>> +
>> + return fd->allow_bitmap[item_index] & mask;
>
> See earlier comment on the size of "allow_bitmap". This code implies the
> size should be 64K bits, but the size is actually 512K bits.
Ditto.
>
>> +}
>> +
>> +static int mshv_vtl_hvcall_call(struct mshv_vtl_hvcall_fd *fd,
>> + struct mshv_vtl_hvcall __user *hvcall_user)
>> +{
>> + struct mshv_vtl_hvcall hvcall;
>> + void *in, *out;
>> + int ret;
>> +
>> + if (copy_from_user(&hvcall, hvcall_user, sizeof(struct mshv_vtl_hvcall)))
>> + return -EFAULT;
>> + if (hvcall.input_size > HV_HYP_PAGE_SIZE)
>> + return -EINVAL;
>> + if (hvcall.output_size > HV_HYP_PAGE_SIZE)
>> + return -EINVAL;
>> +
>> + /*
>> + * By default, all hypercalls are not allowed.
>> + * The user mode code has to set up the allow bitmap once.
>> + */
>> +
>> + if (!mshv_vtl_hvcall_is_allowed(fd, hvcall.control & 0xFFFF)) {
>> + dev_err(fd->dev->this_device,
>> + "Hypercall with control data %#llx isn't allowed\n",
>> + hvcall.control);
>> + return -EPERM;
>> + }
>> +
>> + /*
>> + * This may create a problem for Confidential VM (CVM) usecase where we need to use
>> + * Hyper-V driver allocated per-cpu input and output pages (hyperv_pcpu_input_arg and
>> + * hyperv_pcpu_output_arg) for making a hypervisor call.
>> + *
>> + * TODO: Take care of this when CVM support is added.
>> + */
>> + in = (void *)__get_free_page(GFP_KERNEL);
>> + out = (void *)__get_free_page(GFP_KERNEL);
>> +
>> + if (copy_from_user(in, (void __user *)hvcall.input_ptr, hvcall.input_size)) {
>> + ret = -EFAULT;
>> + goto free_pages;
>> + }
>> +
>> + hvcall.status = hv_do_hypercall(hvcall.control, in, out);
>> +
>> + if (copy_to_user((void __user *)hvcall.output_ptr, out, hvcall.output_size)) {
>> + ret = -EFAULT;
>> + goto free_pages;
>> + }
>> + ret = put_user(hvcall.status, &hvcall_user->status);
>> +free_pages:
>> + free_page((unsigned long)in);
>> + free_page((unsigned long)out);
>> +
>> + return ret;
>> +}
>> +
>> +static long mshv_vtl_hvcall_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>> +{
>> + struct mshv_vtl_hvcall_fd *fd = f->private_data;
>> +
>> + switch (cmd) {
>> + case MSHV_HVCALL_SETUP:
>> + return mshv_vtl_hvcall_setup(fd, (struct mshv_vtl_hvcall_setup __user *)arg);
>> + case MSHV_HVCALL:
>> + return mshv_vtl_hvcall_call(fd, (struct mshv_vtl_hvcall __user *)arg);
>> + default:
>> + break;
>> + }
>> +
>> + return -ENOIOCTLCMD;
>> +}
>> +
>> +static const struct file_operations mshv_vtl_hvcall_file_ops = {
>> + .owner = THIS_MODULE,
>> + .open = mshv_vtl_hvcall_open,
>> + .release = mshv_vtl_hvcall_release,
>> + .unlocked_ioctl = mshv_vtl_hvcall_ioctl,
>> +};
>> +
>> +static struct miscdevice mshv_vtl_hvcall = {
>
> Unfortunately, this global variable name ("mshv_vtl_hvcall") is the
> same as the name of struct mshv_vtl_hvcall in include/uapi/linux/mshv.h.
> Even though it works, could some other name be used here to avoid
> any ambiguity?
Will change for this and mshv_vtl_hvcall_setup as well.
>
>> + .name = "mshv_hvcall",
>> + .nodename = "mshv_hvcall",
>> + .fops = &mshv_vtl_hvcall_file_ops,
>> + .mode = 0600,
>> + .minor = MISC_DYNAMIC_MINOR,
>> +};
>> +
>> +static int mshv_vtl_low_open(struct inode *inodep, struct file *filp)
>> +{
>> + pid_t pid = task_pid_vnr(current);
>> + uid_t uid = current_uid().val;
>> + int ret = 0;
>> +
>> + pr_debug("%s: Opening VTL low, task group %d, uid %d\n", __func__, pid, uid);
>> +
>> + if (capable(CAP_SYS_ADMIN)) {
>> + filp->private_data = inodep;
>> + } else {
>> + pr_err("%s: VTL low open failed: CAP_SYS_ADMIN required. task group %d, uid %d",
>> + __func__, pid, uid);
>> + ret = -EPERM;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static bool can_fault(struct vm_fault *vmf, unsigned long size, pfn_t *pfn)
>> +{
>> + unsigned long mask = size - 1;
>> + unsigned long start = vmf->address & ~mask;
>> + unsigned long end = start + size;
>> + bool is_valid;
>> +
>> + is_valid = (vmf->address & mask) == ((vmf->pgoff << PAGE_SHIFT) & mask) &&
>> + start >= vmf->vma->vm_start &&
>> + end <= vmf->vma->vm_end;
>> +
>> + if (is_valid)
>> + *pfn = __pfn_to_pfn_t(vmf->pgoff & ~(mask >> PAGE_SHIFT), PFN_DEV | PFN_MAP);
>> +
>> + return is_valid;
>> +}
>> +
>> +static vm_fault_t mshv_vtl_low_huge_fault(struct vm_fault *vmf, unsigned int order)
>> +{
>> + pfn_t pfn;
>> + int ret = VM_FAULT_FALLBACK;
>> +
>> + switch (order) {
>> + case 0:
>> + pfn = __pfn_to_pfn_t(vmf->pgoff, PFN_DEV | PFN_MAP);
>> + return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
>> +
>> + case PMD_ORDER:
>> + if (can_fault(vmf, PMD_SIZE, &pfn))
>> + ret = vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);
>> + return ret;
>> +
>> + case PUD_ORDER:
>> + if (can_fault(vmf, PUD_SIZE, &pfn))
>> + ret = vmf_insert_pfn_pud(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);
>> + return ret;
>> +
>> + default:
>> + return VM_FAULT_SIGBUS;
>> + }
>> +}
>
> There's an 'mm' patch set currently in linux-next that eliminates pfn_t, PFN_DEV, and
> PFN_MAP. Assuming the patch set goes into 6.17, the fault handling code here will need
> some rework. See
> https://lore.kernel.org/lkml/cover.176965585864cb8d2cf41464b44dcc0471e643a0.1750323463.git-series.apopple@nvidia.com/
>
Thanks for sharing this, will take care of it.
>> +
>> +static vm_fault_t mshv_vtl_low_fault(struct vm_fault *vmf)
>> +{
>> + return mshv_vtl_low_huge_fault(vmf, 0);
>> +}
>> +
>> +static const struct vm_operations_struct mshv_vtl_low_vm_ops = {
>> + .fault = mshv_vtl_low_fault,
>> + .huge_fault = mshv_vtl_low_huge_fault,
>> +};
>> +
>> +static int mshv_vtl_low_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + vma->vm_ops = &mshv_vtl_low_vm_ops;
>> + vm_flags_set(vma, VM_HUGEPAGE | VM_MIXEDMAP);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations mshv_vtl_low_file_ops = {
>> + .owner = THIS_MODULE,
>> + .open = mshv_vtl_low_open,
>> + .mmap = mshv_vtl_low_mmap,
>> +};
>> +
>> +static struct miscdevice mshv_vtl_low = {
>> + .name = "mshv_vtl_low",
>> + .nodename = "mshv_vtl_low",
>> + .fops = &mshv_vtl_low_file_ops,
>> + .mode = 0600,
>> + .minor = MISC_DYNAMIC_MINOR,
>> +};
>> +
>> +static int __init mshv_vtl_init(void)
>> +{
>> + int ret;
>> + struct device *dev = mshv_dev.this_device;
>> +
>> + /*
>> + * This creates /dev/mshv which provides functionality to create VTLs and partitions.
>> + */
>> + ret = misc_register(&mshv_dev);
>> + if (ret) {
>> + dev_err(dev, "mshv device register failed: %d\n", ret);
>> + goto free_dev;
>> + }
>> +
>> + tasklet_init(&msg_dpc, mshv_vtl_sint_on_msg_dpc, 0);
>> + init_waitqueue_head(&fd_wait_queue);
>> +
>> + if (mshv_vtl_get_vsm_regs()) {
>> + dev_emerg(dev, "Unable to get VSM capabilities !!\n");
>> + ret = -ENODEV;
>> + goto free_dev;
>> + }
>> + if (mshv_vtl_configure_vsm_partition(dev)) {
>> + dev_emerg(dev, "VSM configuration failed !!\n");
>> + ret = -ENODEV;
>> + goto free_dev;
>> + }
>> +
>> + ret = hv_vtl_setup_synic();
>> + if (ret)
>> + goto free_dev;
>> +
>> + /*
>> + * mshv_sint device adds VMBus relay ioctl support.
>> + * This provides a channel for VTL0 to communicate with VTL2.
>> + */
>> + ret = misc_register(&mshv_vtl_sint_dev);
>> + if (ret)
>> + goto free_synic;
>> +
>> + /*
>> + * mshv_hvcall device adds interface to enable userspace for direct hypercalls support.
>> + */
>> + ret = misc_register(&mshv_vtl_hvcall);
>> + if (ret)
>> + goto free_sint;
>> +
>> + /*
>> + * mshv_vtl_low device is used to map VTL0 address space to a user-mode process in VTL2.
>> + * It implements mmap() to allow a user-mode process in VTL2 to map to the address of VTL0.
>> + */
>> + ret = misc_register(&mshv_vtl_low);
>> + if (ret)
>> + goto free_hvcall;
>> +
>> + /*
>> + * "mshv vtl mem dev" device is later used to setup VTL0 memory.
>> + */
>> + mem_dev = kzalloc(sizeof(*mem_dev), GFP_KERNEL);
>> + if (!mem_dev) {
>> + ret = -ENOMEM;
>> + goto free_low;
>> + }
>> +
>> + mutex_init(&mshv_vtl_poll_file_lock);
>> +
>> + device_initialize(mem_dev);
>> + dev_set_name(mem_dev, "mshv vtl mem dev");
>> + ret = device_add(mem_dev);
>> + if (ret) {
>> + dev_err(dev, "mshv vtl mem dev add: %d\n", ret);
>> + goto free_mem;
>> + }
>> +
>> + return 0;
>> +
>> +free_mem:
>> + kfree(mem_dev);
>> +free_low:
>> + misc_deregister(&mshv_vtl_low);
>> +free_hvcall:
>> + misc_deregister(&mshv_vtl_hvcall);
>> +free_sint:
>> + misc_deregister(&mshv_vtl_sint_dev);
>> +free_synic:
>> + hv_vtl_remove_synic();
>> +free_dev:
>> + misc_deregister(&mshv_dev);
>> +
>> + return ret;
>> +}
>> +
>> +static void __exit mshv_vtl_exit(void)
>> +{
>> + device_del(mem_dev);
>> + kfree(mem_dev);
>> + misc_deregister(&mshv_vtl_low);
>> + misc_deregister(&mshv_vtl_hvcall);
>> + misc_deregister(&mshv_vtl_sint_dev);
>> + hv_vtl_remove_synic();
>> + misc_deregister(&mshv_dev);
>> +}
>> +
>> +module_init(mshv_vtl_init);
>> +module_exit(mshv_vtl_exit);
>> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
>> index 1be7f6a02304..cc9260c37c49 100644
>> --- a/include/hyperv/hvgdk_mini.h
>> +++ b/include/hyperv/hvgdk_mini.h
>> @@ -882,6 +882,23 @@ struct hv_get_vp_from_apic_id_in {
>> u32 apic_ids[];
>> } __packed;
>>
>> +union hv_register_vsm_partition_config {
>> + u64 as_uint64;
>> + struct {
>> + u64 enable_vtl_protection : 1;
>> + u64 default_vtl_protection_mask : 4;
>> + u64 zero_memory_on_reset : 1;
>> + u64 deny_lower_vtl_startup : 1;
>> + u64 intercept_acceptance : 1;
>> + u64 intercept_enable_vtl_protection : 1;
>> + u64 intercept_vp_startup : 1;
>> + u64 intercept_cpuid_unimplemented : 1;
>> + u64 intercept_unrecoverable_exception : 1;
>> + u64 intercept_page : 1;
>> + u64 mbz : 51;
>> + };
>
> Add __packed since this is a binary interface with Hyper-V.
> All the other register definitions have __packed.
Acked.
>
>> +};
>> +
>> struct hv_nested_enlightenments_control {
>> struct {
>> u32 directhypercall : 1;
>> @@ -1004,6 +1021,70 @@ enum hv_register_name {
>>
>> /* VSM */
>> HV_REGISTER_VSM_VP_STATUS = 0x000D0003,
>> +
>> + /* Synthetic VSM registers */
>> + HV_REGISTER_VSM_CODE_PAGE_OFFSETS = 0x000D0002,
>> + HV_REGISTER_VSM_CAPABILITIES = 0x000D0006,
>> + HV_REGISTER_VSM_PARTITION_CONFIG = 0x000D0007,
>> +
>> +#if defined(CONFIG_X86)
>> + /* X64 Debug Registers */
>> + HV_X64_REGISTER_DR0 = 0x00050000,
>> + HV_X64_REGISTER_DR1 = 0x00050001,
>> + HV_X64_REGISTER_DR2 = 0x00050002,
>> + HV_X64_REGISTER_DR3 = 0x00050003,
>> + HV_X64_REGISTER_DR6 = 0x00050004,
>> + HV_X64_REGISTER_DR7 = 0x00050005,
>> +
>> + /* X64 Cache control MSRs */
>> + HV_X64_REGISTER_MSR_MTRR_CAP = 0x0008000D,
>> + HV_X64_REGISTER_MSR_MTRR_DEF_TYPE = 0x0008000E,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE0 = 0x00080010,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE1 = 0x00080011,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE2 = 0x00080012,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE3 = 0x00080013,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE4 = 0x00080014,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE5 = 0x00080015,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE6 = 0x00080016,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE7 = 0x00080017,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE8 = 0x00080018,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASE9 = 0x00080019,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASEA = 0x0008001A,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASEB = 0x0008001B,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASEC = 0x0008001C,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASED = 0x0008001D,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASEE = 0x0008001E,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_BASEF = 0x0008001F,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK0 = 0x00080040,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK1 = 0x00080041,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK2 = 0x00080042,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK3 = 0x00080043,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK4 = 0x00080044,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK5 = 0x00080045,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK6 = 0x00080046,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK7 = 0x00080047,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK8 = 0x00080048,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASK9 = 0x00080049,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKA = 0x0008004A,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKB = 0x0008004B,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKC = 0x0008004C,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKD = 0x0008004D,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKE = 0x0008004E,
>> + HV_X64_REGISTER_MSR_MTRR_PHYS_MASKF = 0x0008004F,
>> + HV_X64_REGISTER_MSR_MTRR_FIX64K00000 = 0x00080070,
>> + HV_X64_REGISTER_MSR_MTRR_FIX16K80000 = 0x00080071,
>> + HV_X64_REGISTER_MSR_MTRR_FIX16KA0000 = 0x00080072,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KC0000 = 0x00080073,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KC8000 = 0x00080074,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KD0000 = 0x00080075,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KD8000 = 0x00080076,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KE0000 = 0x00080077,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KE8000 = 0x00080078,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KF0000 = 0x00080079,
>> + HV_X64_REGISTER_MSR_MTRR_FIX4KF8000 = 0x0008007A,
>> +
>> + HV_X64_REGISTER_REG_PAGE = 0x0009001C,
>> +#endif
>> };
>>
>> /*
>> diff --git a/include/hyperv/hvhdk.h b/include/hyperv/hvhdk.h
>> index b4067ada02cf..c6a62ec9f6da 100644
>> --- a/include/hyperv/hvhdk.h
>> +++ b/include/hyperv/hvhdk.h
>> @@ -479,6 +479,7 @@ struct hv_connection_info {
>> #define HV_EVENT_FLAGS_COUNT (256 * 8)
>> #define HV_EVENT_FLAGS_BYTE_COUNT (256)
>> #define HV_EVENT_FLAGS32_COUNT (256 / sizeof(u32))
>> +#define HV_EVENT_FLAGS_LONG_COUNT (HV_EVENT_FLAGS_BYTE_COUNT / sizeof(u64))
>>
>> /* linux side we create long version of flags to use long bit ops on flags */
>> #define HV_EVENT_FLAGS_UL_COUNT (256 / sizeof(ulong))
>> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
>> index 876bfe4e4227..a218536eaec1 100644
>> --- a/include/uapi/linux/mshv.h
>> +++ b/include/uapi/linux/mshv.h
>> @@ -288,4 +288,86 @@ struct mshv_get_set_vp_state {
>> * #define MSHV_ROOT_HVCALL _IOWR(MSHV_IOCTL, 0x07, struct mshv_root_hvcall)
>> */
>>
>> +/* Structure definitions, macros and IOCTLs for mshv_vtl */
>> +
>> +#define MSHV_CAP_CORE_API_STABLE 0x0
>> +#define MSHV_CAP_REGISTER_PAGE 0x1
>> +#define MSHV_CAP_VTL_RETURN_ACTION 0x2
>> +#define MSHV_CAP_DR6_SHARED 0x3
>> +#define MSHV_MAX_RUN_MSG_SIZE 256
>> +
>> +#define MSHV_VP_MAX_REGISTERS 128
>> +
>> +struct mshv_vp_registers {
>> + __u32 count; /* at most MSHV_VP_MAX_REGISTERS */
>> + __u32 reserved; /* Reserved for alignment or future use */
>> + __u64 regs_ptr; /* pointer to struct hv_register_assoc */
>> +};
>> +
>> +struct mshv_vtl_set_eventfd {
>> + __s32 fd;
>> + __u32 flag;
>> +};
>> +
>> +struct mshv_vtl_signal_event {
>> + __u32 connection_id;
>> + __u32 flag;
>> +};
>> +
>> +struct mshv_vtl_sint_post_msg {
>> + __u64 message_type;
>> + __u32 connection_id;
>> + __u32 payload_size; /* Must not exceed HV_MESSAGE_PAYLOAD_BYTE_COUNT */
>> + __u64 payload_ptr; /* pointer to message payload (bytes) */
>> +};
>> +
>> +struct mshv_vtl_ram_disposition {
>> + __u64 start_pfn;
>> + __u64 last_pfn;
>> +};
>> +
>> +struct mshv_vtl_set_poll_file {
>> + __u32 cpu;
>> + __u32 fd;
>> +};
>> +
>> +struct mshv_vtl_hvcall_setup {
>> + __u64 bitmap_size;
>
> What are the units of "bitmap_size"? Bits? Bytes? u64?
It would be length of bitmap array.
>
>> + __u64 allow_bitmap_ptr; /* pointer to __u64 */
>> +};
>> +
>> +struct mshv_vtl_hvcall {
>> + __u64 control; /* Hypercall control code */
>> + __u64 input_size; /* Size of the input data */
>> + __u64 input_ptr; /* Pointer to the input struct */
>> + __u64 status; /* Status of the hypercall (output) */
>> + __u64 output_size; /* Size of the output data */
>> + __u64 output_ptr; /* Pointer to the output struct */
>> +};
>> +
>> +struct mshv_sint_mask {
>> + __u8 mask;
>> + __u8 reserved[7];
>> +};
>> +
>> +/* /dev/mshv device IOCTL */
>> +#define MSHV_CHECK_EXTENSION _IOW(MSHV_IOCTL, 0x00, __u32)
>> +
>> +/* vtl device */
>> +#define MSHV_CREATE_VTL _IOR(MSHV_IOCTL, 0x1D, char)
>> +#define MSHV_ADD_VTL0_MEMORY _IOW(MSHV_IOCTL, 0x21, struct mshv_vtl_ram_disposition)
>> +#define MSHV_SET_POLL_FILE _IOW(MSHV_IOCTL, 0x25, struct mshv_vtl_set_poll_file)
>> +#define MSHV_RETURN_TO_LOWER_VTL _IO(MSHV_IOCTL, 0x27)
>> +#define MSHV_GET_VP_REGISTERS _IOWR(MSHV_IOCTL, 0x05, struct mshv_vp_registers)
>> +#define MSHV_SET_VP_REGISTERS _IOW(MSHV_IOCTL, 0x06, struct mshv_vp_registers)
>> +
>> +/* VMBus device IOCTLs */
>> +#define MSHV_SINT_SIGNAL_EVENT _IOW(MSHV_IOCTL, 0x22, struct mshv_vtl_signal_event)
>> +#define MSHV_SINT_POST_MESSAGE _IOW(MSHV_IOCTL, 0x23, struct mshv_vtl_sint_post_msg)
>> +#define MSHV_SINT_SET_EVENTFD _IOW(MSHV_IOCTL, 0x24, struct mshv_vtl_set_eventfd)
>> +#define MSHV_SINT_PAUSE_MESSAGE_STREAM _IOW(MSHV_IOCTL, 0x25, struct mshv_sint_mask)
>> +
>> +/* hv_hvcall device */
>> +#define MSHV_HVCALL_SETUP _IOW(MSHV_IOCTL, 0x1E, struct mshv_vtl_hvcall_setup)
>> +#define MSHV_HVCALL _IOWR(MSHV_IOCTL, 0x1F, struct mshv_vtl_hvcall)
>> #endif
>> --
>> 2.34.1
>>
>
Thanks Michael for thoroughly reviewing the patch and sharing your
valuable comments.
I will wait for your comment on vmbus_chan_sched query for sending the
next patch, or anything else if you have in mind.
Regards,
Naman
^ permalink raw reply
* [PATCH v4 0/7] hyperv: Introduce new way to manage hypercall args
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
From: Michael Kelley <mhklinux@outlook.com>
This patch set introduces a new way to manage the use of the per-vCPU
memory that is usually the input and output arguments to Hyper-V
hypercalls. Current code allocates the "hyperv_pcpu_input_arg", and in
some configurations, the "hyperv_pcpu_output_arg". Each is a 4 KiB
page of memory allocated per-vCPU. A hypercall call site disables
interrupts, then uses this memory to set up the input parameters for
the hypercall, read the output results after hypercall execution, and
re-enable interrupts. The open coding of these steps has led to
inconsistencies, and in some cases, violation of the generic
requirements for the hypercall input and output as described in the
Hyper-V Top Level Functional Spec (TLFS)[1]. This patch set introduces
a new family of inline functions to replace the open coding. The new
functions encapsulate key aspects of the use of per-vCPU memory for
hypercall input and output, and ensure that the TLFS requirements are
met (max size of 1 page each for input and output, no overlap of input
and output, aligned to 8 bytes, etc.).
With this change, hypercall call sites no longer directly access
"hyperv_pcpu_input_arg" and "hyperv_pcpu_output_arg". Instead, one of
a family of new functions provides the per-vCPU memory that a hypercall
call site uses to set up hypercall input and output areas.
Conceptually, there is no longer a difference between the "per-vCPU
input page" and "per-vCPU output page". Only a single per-vCPU page is
allocated, and it is used to provide both hypercall input and output.
All current hypercalls can fit their input and output within that single
page, though the new code allows easy changing to two pages should a
future hypercall require a full page for each of the input and output.
The new functions always zero the fixed-size portion of the hypercall
input area (but not any array portion -- see below) so that
uninitialized memory isn't inadvertently passed to the hypercall.
Current open-coded hypercall call sites are inconsistent on this point,
and use of the new functions addresses that inconsistency. The output
area is not zero'ed by the new code as it is Hyper-V's responsibility
to provide legal output.
When the input or output (or both) contain an array, the new code
calculates and returns how many array entries fit within the per-vCPU
memory page, which is effectively the "batch size" for the hypercall
processing multiple entries. This batch size can then be used in the
hypercall control word to specify the repetition count. This
calculation of the batch size replaces current open coding of the
batch size, which is prone to errors. Note that the array portion of
the input area is *not* zero'ed. The arrays are almost always 64-bit
GPAs or something similar, and zero'ing that much memory seems
wasteful at runtime when it will all be overwritten. The hypercall
call site is responsible for ensuring that no part of the array is
left uninitialized (just as with current code).
The new family of functions is realized as a single inline function
that handles the most complex case, which is a hypercall with input
and output, both of which contain arrays. Simpler cases are mapped to
this most complex case with #define wrappers that provide zero or NULL
for some arguments. Several of the arguments to this new function
must be compile-time constants generated by "sizeof()" expressions.
As such, most of the code in the new function is evaluated by the
compiler, with the result that the runtime code paths are no longer
than with the current open coding. An exception is the new code
generated to zero the fixed-size portion of the input area in cases
where it was not previously done.
Use of the new function typically (but not always) saves a few lines
of code at each hypercall call site. This is traded off against the
lines of code added for the new functions. With code currently
upstream, the net is an add of about 20 lines of code and comments.
A couple hypercall call sites have requirements that are not 100%
handled by the new function. These still require some manual open-
coded adjustment or open-coded batch size calculations -- see the
individual patches in this series. Suggestions on how to do better
are welcome.
The patches in the series do the following:
Patch 1: Introduce the new family of functions for assigning hypercall
input and output arguments.
Patch 2 to 6: Change existing hypercall call sites to use one of the new
functions. In some cases, tweaks to the hypercall argument data
structures are necessary, but these tweaks are making the data
structures more consistent with the overall pattern. These
5 patches are independent of each other, and can go in any
order. The breakup into 5 patches is for ease of review.
Patch 7: Update the name of the variable used to hold the per-vCPU memory
used for hypercall arguments. Remove code for managing the
per-vCPU output page.
The new code compiles and runs successfully on x86 and arm64. However,
basic smoke tests cover only a limited number of hypercall call sites
that have been modified. I don't have the hardware or Hyper-V
configurations needed to test running in the Hyper-V root partition
or running in a VTL other than VTL 0. The related hypercall call sites
still need to be tested to make sure I didn't break anything. Hopefully
someone with the necessary configurations and Hyper-V versions can
help with that testing.
For gcc 9.4.0, I've looked at the generated code for a couple of
hypercall call sites on both x86 and arm64 to ensure that it boils
down to the equivalent of the current open coding. I have not looked
at the generated code for later gcc versions or for Clang/LLVM, but
there's no reason to expect something worse as the code isn't doing
anything tricky.
This patch set is built against linux-next20250716.
[1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/tlfs
Michael Kelley (7):
Drivers: hv: Introduce hv_setup_*() functions for hypercall arguments
x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 1
x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 2
Drivers: hv: Use hv_setup_*() to set up hypercall arguments
PCI: hv: Use hv_setup_*() to set up hypercall arguments
Drivers: hv: Use hv_setup_*() to set up hypercall arguments for mshv
code
Drivers: hv: Replace hyperv_pcpu_input/output_arg with hyperv_pcpu_arg
arch/x86/hyperv/hv_apic.c | 10 +--
arch/x86/hyperv/hv_init.c | 12 ++-
arch/x86/hyperv/hv_vtl.c | 3 +-
arch/x86/hyperv/irqdomain.c | 17 ++--
arch/x86/hyperv/ivm.c | 18 ++---
arch/x86/hyperv/mmu.c | 19 ++---
arch/x86/hyperv/nested.c | 14 ++--
drivers/hv/hv.c | 6 +-
drivers/hv/hv_balloon.c | 8 +-
drivers/hv/hv_common.c | 64 +++++----------
drivers/hv/hv_proc.c | 23 +++---
drivers/hv/hyperv_vmbus.h | 2 +-
drivers/hv/mshv_common.c | 31 +++----
drivers/hv/mshv_root_hv_call.c | 121 +++++++++++-----------------
drivers/hv/mshv_root_main.c | 5 +-
drivers/pci/controller/pci-hyperv.c | 18 ++---
include/asm-generic/mshyperv.h | 106 +++++++++++++++++++++++-
include/hyperv/hvgdk_mini.h | 4 +-
18 files changed, 251 insertions(+), 230 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v4 1/7] Drivers: hv: Introduce hv_setup_*() functions for hypercall arguments
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@outlook.com>
Current code allocates the "hyperv_pcpu_input_arg", and in
some configurations, the "hyperv_pcpu_output_arg". Each is a 4 KiB
page of memory allocated per-vCPU. A hypercall call site disables
interrupts, then uses this memory to set up the input parameters for
the hypercall, read the output results after hypercall execution, and
re-enable interrupts. The open coding of these steps leads to
inconsistencies, and in some cases, violation of the generic
requirements for the hypercall input and output as described in the
Hyper-V Top Level Functional Spec (TLFS)[1].
To reduce these kinds of problems, introduce a family of inline
functions to replace the open coding. The functions provide a new way
to manage the use of this per-vCPU memory that is usually the input and
output arguments to Hyper-V hypercalls. The functions encapsulate
key aspects of the usage and ensure that the TLFS requirements are
met (max size of 1 page each for input and output, no overlap of
input and output, aligned to 8 bytes, etc.). Conceptually, there
is no longer a difference between the "per-vCPU input page" and
"per-vCPU output page". Only a single per-vCPU page is allocated, and
it provides both hypercall input and output memory. All current
hypercalls can fit their input and output within that single page,
though the new code allows easy changing to two pages should a future
hypercall require a full page for each of the input and output.
The new functions always zero the fixed-size portion of the hypercall
input area so that uninitialized memory is not inadvertently passed
to the hypercall. Current open-coded hypercall call sites are
inconsistent on this point, and use of the new functions addresses
that inconsistency. The output area is not zero'ed by the new code
as it is Hyper-V's responsibility to provide legal output.
When the input or output (or both) contain an array, the new functions
calculate and return how many array entries fit within the per-vCPU
memory page, which is effectively the "batch size" for the hypercall
processing multiple entries. This batch size can then be used in the
hypercall control word to specify the repetition count. This
calculation of the batch size replaces current open coding of the
batch size, which is prone to errors. Note that the array portion of
the input area is *not* zero'ed. The arrays are almost always 64-bit
GPAs or something similar, and zero'ing that much memory seems
wasteful at runtime when it will all be overwritten. The hypercall
call site is responsible for ensuring that no part of the array is
left uninitialized (just as with current code).
The new functions are realized as a single inline function that
handles the most complex case, which is a hypercall with input
and output, both of which contain arrays. Simpler cases are mapped to
this most complex case with #define wrappers that provide zero or NULL
for some arguments. Several of the arguments to this new function
must be compile-time constants generated by "sizeof()"
expressions. As such, most of the code in the new function can be
evaluated by the compiler, with the result that the code paths are
no longer than with the current open coding. The one exception is
new code generated to zero the fixed-size portion of the input area
in cases where it is not currently done.
[1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/tlfs
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Reviewed-by: Nuno Das Neves <nunodasneves@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 v3:
* Added wrapper #define hv_hvcall_in_batch_size() to get the batch size
without setting up hypercall input/output parameters. This call can be
used when the batch size is needed for validation checks or memory
allocations prior to disabling interrupts.
Changes in v2:
* Added comment that hv_hvcall_inout_array() should always be called with
interrupts disabled because it is returning pointers to per-cpu memory
[Nuno Das Neves]
include/asm-generic/mshyperv.h | 106 +++++++++++++++++++++++++++++++++
1 file changed, 106 insertions(+)
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index a729b77983fa..040c4650f411 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -151,6 +151,112 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
return status;
}
+/*
+ * Hypercall input and output argument setup
+ */
+
+/* Temporary mapping to be removed at the end of the patch series */
+#define hyperv_pcpu_arg hyperv_pcpu_input_arg
+
+/*
+ * Allocate one page that is shared between input and output args, which is
+ * sufficient for all current hypercalls. If a future hypercall requires
+ * more space, change this value to "2" and everything will work.
+ */
+#define HV_HVCALL_ARG_PAGES 1
+
+/*
+ * Allocate space for hypercall input and output arguments from the
+ * pre-allocated per-cpu hyperv_pcpu_args page(s). A NULL value for the input
+ * or output indicates to allocate no space for that argument. For input and
+ * for output, specify the size of the fixed portion, and the size of an
+ * element in a variable size array. A zero value for entry_size indicates
+ * there is no array. The fixed size space for the input is zero'ed.
+ *
+ * When variable size arrays are present, the function returns the number of
+ * elements (i.e, the batch size) that fit in the available space.
+ *
+ * The four "size" arguments must be constants so the compiler can do most of
+ * the calculations. Then the generated inline code is no larger than if open
+ * coding the access to the hyperv_pcpu_arg and doing memset() on the input.
+ *
+ * This function must be called with interrupts disabled so the thread is not
+ * rescheduled onto another vCPU while accessing the per-cpu args page.
+ */
+static inline int hv_setup_inout_array(void *input, u32 in_size, u32 in_entry_size,
+ void *output, u32 out_size, u32 out_entry_size)
+{
+ u32 in_batch_count = 0, out_batch_count = 0, batch_count;
+ u32 in_total_size, out_total_size, offset;
+ u32 batch_space = HV_HYP_PAGE_SIZE * HV_HVCALL_ARG_PAGES;
+ void *space;
+
+ /*
+ * If input and output have arrays, allocate half the space to input
+ * and half to output. If only input has an array, the array can use
+ * all the space except for the fixed size output (but not to exceed
+ * one page), and vice versa.
+ */
+ if (in_entry_size && out_entry_size)
+ batch_space = batch_space / 2;
+ else if (in_entry_size)
+ batch_space = min(HV_HYP_PAGE_SIZE, batch_space - out_size);
+ else if (out_entry_size)
+ batch_space = min(HV_HYP_PAGE_SIZE, batch_space - in_size);
+
+ if (in_entry_size)
+ in_batch_count = (batch_space - in_size) / in_entry_size;
+ if (out_entry_size)
+ out_batch_count = (batch_space - out_size) / out_entry_size;
+
+ /*
+ * If input and output have arrays, use the smaller of the two batch
+ * counts, in case they are different. If only one has an array, use
+ * that batch count. batch_count will be zero if neither has an array.
+ */
+ if (in_batch_count && out_batch_count)
+ batch_count = min(in_batch_count, out_batch_count);
+ else
+ batch_count = in_batch_count | out_batch_count;
+
+ in_total_size = ALIGN(in_size + (in_entry_size * batch_count), 8);
+ out_total_size = ALIGN(out_size + (out_entry_size * batch_count), 8);
+
+ space = *this_cpu_ptr(hyperv_pcpu_arg);
+ if (input) {
+ *(void **)input = space;
+ if (space)
+ /* Zero the fixed size portion, not any array portion */
+ memset(space, 0, ALIGN(in_size, 8));
+ }
+
+ if (output) {
+ if (in_total_size + out_total_size <= HV_HYP_PAGE_SIZE) {
+ offset = in_total_size;
+ } else {
+ offset = HV_HYP_PAGE_SIZE;
+ /* Need more than 1 page, but only 1 was allocated */
+ BUILD_BUG_ON(HV_HVCALL_ARG_PAGES == 1);
+ }
+ *(void **)output = space + offset;
+ }
+
+ return batch_count;
+}
+
+/* Wrappers for some of the simpler cases with only input, or with no arrays */
+#define hv_setup_in(input, in_size) \
+ hv_setup_inout_array(input, in_size, 0, NULL, 0, 0)
+
+#define hv_setup_inout(input, in_size, output, out_size) \
+ hv_setup_inout_array(input, in_size, 0, output, out_size, 0)
+
+#define hv_setup_in_array(input, in_size, in_entry_size) \
+ hv_setup_inout_array(input, in_size, in_entry_size, NULL, 0, 0)
+
+#define hv_get_input_batch_size(in_size, in_entry_size) \
+ hv_setup_inout_array(NULL, in_size, in_entry_size, NULL, 0, 0)
+
/* Generate the guest OS identifier as described in the Hyper-V TLFS */
static inline u64 hv_generate_guest_id(u64 kernel_version)
{
--
2.25.1
^ permalink raw reply related
* [PATCH v4 2/7] x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 1
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@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 <mhklinux@outlook.com>
Reviewed-by: Nuno Das Neves <nunodasneves@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]
arch/x86/hyperv/hv_apic.c | 10 ++++------
arch/x86/hyperv/hv_init.c | 6 ++----
arch/x86/hyperv/hv_vtl.c | 3 +--
arch/x86/hyperv/irqdomain.c | 17 ++++++++++-------
4 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
index bfde0a3498b9..bafb5dceb5d6 100644
--- a/arch/x86/hyperv/hv_apic.c
+++ b/arch/x86/hyperv/hv_apic.c
@@ -109,21 +109,19 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
{
struct hv_send_ipi_ex *ipi_arg;
unsigned long flags;
- int nr_bank = 0;
+ int batch_size, nr_bank = 0;
u64 status = HV_STATUS_INVALID_PARAMETER;
if (!(ms_hyperv.hints & HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED))
return false;
local_irq_save(flags);
- ipi_arg = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
+ batch_size = hv_setup_in_array(&ipi_arg, sizeof(*ipi_arg),
+ sizeof(ipi_arg->vp_set.bank_contents[0]));
if (unlikely(!ipi_arg))
goto ipi_mask_ex_done;
ipi_arg->vector = vector;
- ipi_arg->reserved = 0;
- ipi_arg->vp_set.valid_bank_mask = 0;
/*
* Use HV_GENERIC_SET_ALL and avoid converting cpumask to VP_SET
@@ -140,7 +138,7 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
* represented in VP_SET. Return an error and fall back to
* native (architectural) method of sending IPIs.
*/
- if (nr_bank <= 0)
+ if (nr_bank <= 0 || nr_bank > batch_size)
goto ipi_mask_ex_done;
} else {
ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index afdbda2dd7b7..b7a2877c2a92 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -685,13 +685,11 @@ int hv_apicid_to_vp_index(u32 apic_id)
local_irq_save(irq_flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(input, 0, sizeof(*input));
+ hv_setup_inout_array(&input, sizeof(*input), sizeof(input->apic_ids[0]),
+ &output, 0, sizeof(*output));
input->partition_id = HV_PARTITION_ID_SELF;
input->apic_ids[0] = apic_id;
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
-
control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_INDEX_FROM_APIC_ID;
status = hv_do_hypercall(control, input, output);
ret = output[0];
diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 042e8712d8de..fc523a5096f4 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -131,8 +131,7 @@ static int hv_vtl_bringup_vcpu(u32 target_vp_index, int cpu, u64 eip_ignored)
local_irq_save(irq_flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(input, 0, sizeof(*input));
+ hv_setup_in(&input, sizeof(*input));
input->partition_id = HV_PARTITION_ID_SELF;
input->vp_index = target_vp_index;
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);
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;
--
2.25.1
^ permalink raw reply related
* [PATCH v4 3/7] x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 2
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@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.
For hv_mark_gpa_visibility(), use the computed batch_size instead
of HV_MAX_MODIFY_GPA_REP_COUNT. Also update the associated gpa_page_list[]
field to have zero size, which is more consistent with other array
arguments to hypercalls. Due to the interaction with the calling
hv_vtom_set_host_visibility(), HV_MAX_MODIFY_GPA_REP_COUNT cannot be
completely eliminated without some further restructuring, but that's
for another patch set.
Similarly, for the nested flush functions, update the gpa_list[] to
have zero size. Again, separate restructuring would be required to
completely eliminate the need for HV_MAX_FLUSH_REP_COUNT.
Finally, hyperv_flush_tlb_others_ex() requires special handling
because the input consists of two arrays -- one for the hv_vp_set and
another for the gva list. The batch_size computed by hv_setup_in_array()
is adjusted to account for the number of entries in the hv_vp_set.
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Reviewed-by: Nuno Das Neves <nunodasneves@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:
* In hyperv_flush_tlb_others_ex(), added check of the adjusted
max_gvas to make sure it doesn't go to zero or negative, which would
happen if there is insufficient space to hold the hv_vpset and have
at least one entry in the gva list. 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]
arch/x86/hyperv/ivm.c | 18 +++++++++---------
arch/x86/hyperv/mmu.c | 19 +++++--------------
arch/x86/hyperv/nested.c | 14 +++++---------
include/hyperv/hvgdk_mini.h | 4 ++--
4 files changed, 21 insertions(+), 34 deletions(-)
diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index ade6c665c97e..3084ae8a3eed 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -474,30 +474,30 @@ static int hv_mark_gpa_visibility(u16 count, const u64 pfn[],
{
struct hv_gpa_range_for_visibility *input;
u64 hv_status;
+ int batch_size;
unsigned long flags;
/* no-op if partition isolation is not enabled */
if (!hv_is_isolation_supported())
return 0;
- if (count > HV_MAX_MODIFY_GPA_REP_COUNT) {
- pr_err("Hyper-V: GPA count:%d exceeds supported:%lu\n", count,
- HV_MAX_MODIFY_GPA_REP_COUNT);
+ local_irq_save(flags);
+ batch_size = hv_setup_in_array(&input, sizeof(*input),
+ sizeof(input->gpa_page_list[0]));
+ if (unlikely(!input)) {
+ local_irq_restore(flags);
return -EINVAL;
}
- local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
- if (unlikely(!input)) {
+ if (count > batch_size) {
+ pr_err("Hyper-V: GPA count:%d exceeds supported:%u\n", count,
+ batch_size);
local_irq_restore(flags);
return -EINVAL;
}
input->partition_id = HV_PARTITION_ID_SELF;
input->host_visibility = visibility;
- input->reserved0 = 0;
- input->reserved1 = 0;
memcpy((void *)input->gpa_page_list, pfn, count * sizeof(*pfn));
hv_status = hv_do_rep_hypercall(
HVCALL_MODIFY_SPARSE_GPA_PAGE_HOST_VISIBILITY, count,
diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c
index cfcb60468b01..f1aeb5bdb29a 100644
--- a/arch/x86/hyperv/mmu.c
+++ b/arch/x86/hyperv/mmu.c
@@ -72,7 +72,7 @@ static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
local_irq_save(flags);
- flush = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ max_gvas = hv_setup_in_array(&flush, sizeof(*flush), sizeof(flush->gva_list[0]));
if (unlikely(!flush)) {
local_irq_restore(flags);
@@ -86,13 +86,10 @@ static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
*/
flush->address_space = virt_to_phys(info->mm->pgd);
flush->address_space &= CR3_ADDR_MASK;
- flush->flags = 0;
} else {
- flush->address_space = 0;
flush->flags = HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES;
}
- flush->processor_mask = 0;
if (cpumask_equal(cpus, cpu_present_mask)) {
flush->flags |= HV_FLUSH_ALL_PROCESSORS;
} else {
@@ -139,8 +136,6 @@ static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
* We can flush not more than max_gvas with one hypercall. Flush the
* whole address space if we were asked to do more.
*/
- max_gvas = (PAGE_SIZE - sizeof(*flush)) / sizeof(flush->gva_list[0]);
-
if (info->end == TLB_FLUSH_ALL) {
flush->flags |= HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY;
status = hv_do_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE,
@@ -179,7 +174,7 @@ static u64 hyperv_flush_tlb_others_ex(const struct cpumask *cpus,
if (!(ms_hyperv.hints & HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED))
return HV_STATUS_INVALID_PARAMETER;
- flush = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ max_gvas = hv_setup_in_array(&flush, sizeof(*flush), sizeof(flush->gva_list[0]));
if (info->mm) {
/*
@@ -188,14 +183,10 @@ static u64 hyperv_flush_tlb_others_ex(const struct cpumask *cpus,
*/
flush->address_space = virt_to_phys(info->mm->pgd);
flush->address_space &= CR3_ADDR_MASK;
- flush->flags = 0;
} else {
- flush->address_space = 0;
flush->flags = HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES;
}
- flush->hv_vp_set.valid_bank_mask = 0;
-
flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
info->freed_tables ? NULL : cpu_is_lazy);
@@ -210,10 +201,10 @@ static u64 hyperv_flush_tlb_others_ex(const struct cpumask *cpus,
* of flush->hv_vp_set as part of the fixed size input header.
* So the variable input header size is equal to nr_bank.
*/
- max_gvas =
- (PAGE_SIZE - sizeof(*flush) - nr_bank *
- sizeof(flush->hv_vp_set.bank_contents[0])) /
+ max_gvas -= (nr_bank * sizeof(flush->hv_vp_set.bank_contents[0])) /
sizeof(flush->gva_list[0]);
+ if (max_gvas <= 0)
+ return HV_STATUS_INVALID_PARAMETER;
if (info->end == TLB_FLUSH_ALL) {
flush->flags |= HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY;
diff --git a/arch/x86/hyperv/nested.c b/arch/x86/hyperv/nested.c
index 8ccbb7c4fc27..8eeda2e2ad29 100644
--- a/arch/x86/hyperv/nested.c
+++ b/arch/x86/hyperv/nested.c
@@ -30,15 +30,13 @@ int hyperv_flush_guest_mapping(u64 as)
local_irq_save(flags);
- flush = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
+ hv_setup_in(&flush, sizeof(*flush));
if (unlikely(!flush)) {
local_irq_restore(flags);
goto fault;
}
flush->address_space = as;
- flush->flags = 0;
status = hv_do_hypercall(HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE,
flush, NULL);
@@ -91,25 +89,23 @@ int hyperv_flush_guest_mapping_range(u64 as,
u64 status;
unsigned long flags;
int ret = -ENOTSUPP;
- int gpa_n = 0;
+ int batch_size, gpa_n = 0;
if (!hv_hypercall_pg || !fill_flush_list_func)
goto fault;
local_irq_save(flags);
- flush = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
+ batch_size = hv_setup_in_array(&flush, sizeof(*flush),
+ sizeof(flush->gpa_list[0]));
if (unlikely(!flush)) {
local_irq_restore(flags);
goto fault;
}
flush->address_space = as;
- flush->flags = 0;
-
gpa_n = fill_flush_list_func(flush, data);
- if (gpa_n < 0) {
+ if (gpa_n < 0 || gpa_n > batch_size) {
local_irq_restore(flags);
goto fault;
}
diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index 1be7f6a02304..5590b5fe318c 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h
@@ -557,7 +557,7 @@ union hv_gpa_page_range {
struct hv_guest_mapping_flush_list {
u64 address_space;
u64 flags;
- union hv_gpa_page_range gpa_list[HV_MAX_FLUSH_REP_COUNT];
+ union hv_gpa_page_range gpa_list[];
};
struct hv_tlb_flush { /* HV_INPUT_FLUSH_VIRTUAL_ADDRESS_LIST */
@@ -1244,7 +1244,7 @@ struct hv_gpa_range_for_visibility {
u32 host_visibility : 2;
u32 reserved0 : 30;
u32 reserved1;
- u64 gpa_page_list[HV_MAX_MODIFY_GPA_REP_COUNT];
+ u64 gpa_page_list[];
} __packed;
#if defined(CONFIG_X86)
--
2.25.1
^ permalink raw reply related
* [PATCH v4 4/7] Drivers: hv: Use hv_setup_*() to set up hypercall arguments
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@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 zero'ing of
input fields.
In hv_post_message(), the payload area is treated as an array to
avoid wasting cycles on zero'ing it and then overwriting with
memcpy().
Signed-off-by: Michael Kelley <mhklinux@outlook.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 v3:
* Removed change to definition of struct hv_input_post_message so the
'payload' remains a fixed size array. Adjust hv_post_message() so
that the 'payload' array is not zero'ed. [Nuno Das Neves]
* Added check of the batch size in hv_free_page_report(). [Nuno Das Neves]
* In hv_call_deposit_pages(), use the new hv_hvcall_in_batch_size() to
get the batch size at the start of the function, and check the
'num_pages' input parameter against that batch size instead of against
a separately defined constant. Also use the batch size to compute the
size of the memory allocation. [Nuno Das Neves]
drivers/hv/hv.c | 4 +++-
drivers/hv/hv_balloon.c | 8 ++++----
drivers/hv/hv_common.c | 9 +++------
drivers/hv/hv_proc.c | 23 ++++++++++-------------
4 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index b14c5f9e0ef2..ad063f535f95 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -66,7 +66,9 @@ int hv_post_message(union hv_connection_id connection_id,
if (hv_isolation_type_tdx() && ms_hyperv.paravisor_present)
aligned_msg = this_cpu_ptr(hv_context.cpu_context)->post_msg_page;
else
- aligned_msg = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ hv_setup_in_array(&aligned_msg,
+ offsetof(typeof(*aligned_msg), payload),
+ sizeof(aligned_msg->payload[0]));
aligned_msg->connectionid = connection_id;
aligned_msg->reserved = 0;
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 2b4080e51f97..d9b569b204d2 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1577,21 +1577,21 @@ static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_info,
{
unsigned long flags;
struct hv_memory_hint *hint;
- int i, order;
+ int i, order, batch_size;
u64 status;
struct scatterlist *sg;
- WARN_ON_ONCE(nents > HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES);
WARN_ON_ONCE(sgl->length < (HV_HYP_PAGE_SIZE << page_reporting_order));
local_irq_save(flags);
- hint = *this_cpu_ptr(hyperv_pcpu_input_arg);
+
+ batch_size = hv_setup_in_array(&hint, sizeof(*hint), sizeof(hint->ranges[0]));
if (!hint) {
local_irq_restore(flags);
return -ENOSPC;
}
+ WARN_ON_ONCE(nents > batch_size);
hint->heat_type = HV_EXTMEM_HEAT_HINT_COLD_DISCARD;
- hint->reserved = 0;
for_each_sg(sgl, sg, nents, i) {
union hv_gpa_page_range *range;
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 49898d10faff..ae56397af1ed 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -267,7 +267,7 @@ void __init hv_get_partition_id(void)
u64 status, pt_id;
local_irq_save(flags);
- output = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ hv_setup_inout(NULL, 0, &output, sizeof(*output));
status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output);
pt_id = output->partition_id;
local_irq_restore(flags);
@@ -288,13 +288,10 @@ u8 __init get_vtl(void)
u64 ret;
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
-
- memset(input, 0, struct_size(input, names, 1));
+ hv_setup_inout_array(&input, sizeof(*input), sizeof(input->names[0]),
+ &output, sizeof(*output), sizeof(output->values[0]));
input->partition_id = HV_PARTITION_ID_SELF;
input->vp_index = HV_VP_INDEX_SELF;
- input->input_vtl.as_uint8 = 0;
input->names[0] = HV_REGISTER_VSM_VP_STATUS;
ret = hv_do_hypercall(control, input, output);
diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
index fbb4eb3901bb..bd63830b4141 100644
--- a/drivers/hv/hv_proc.c
+++ b/drivers/hv/hv_proc.c
@@ -9,12 +9,6 @@
#include <linux/export.h>
#include <asm/mshyperv.h>
-/*
- * See struct hv_deposit_memory. The first u64 is partition ID, the rest
- * are GPAs.
- */
-#define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
-
/* Deposits exact number of pages. Must be called with interrupts enabled. */
int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
{
@@ -25,11 +19,13 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
int order;
u64 status;
int ret;
- u64 base_pfn;
+ u64 base_pfn, batch_size;
struct hv_deposit_memory *input_page;
unsigned long flags;
- if (num_pages > HV_DEPOSIT_MAX)
+ batch_size = hv_get_input_batch_size(sizeof(*input_page),
+ sizeof(input_page->gpa_page_list[0]));
+ if (num_pages > batch_size)
return -E2BIG;
if (!num_pages)
return 0;
@@ -40,7 +36,7 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
return -ENOMEM;
pages = page_address(page);
- counts = kcalloc(HV_DEPOSIT_MAX, sizeof(int), GFP_KERNEL);
+ counts = kcalloc(batch_size, sizeof(int), GFP_KERNEL);
if (!counts) {
free_page((unsigned long)pages);
return -ENOMEM;
@@ -74,7 +70,9 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
local_irq_save(flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ /* Batch size is checked at the start of function; no need to repeat */
+ hv_setup_in_array(&input_page, sizeof(*input_page),
+ sizeof(input_page->gpa_page_list[0]));
input_page->partition_id = partition_id;
@@ -126,9 +124,8 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
/* We don't do anything with the output right now */
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->lp_index = lp_index;
input->apic_id = apic_id;
@@ -169,7 +166,7 @@ int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
do {
local_irq_save(irq_flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ hv_setup_in(&input, sizeof(*input));
input->partition_id = partition_id;
input->vp_index = vp_index;
--
2.25.1
^ permalink raw reply related
* [PATCH v4 5/7] PCI: hv: Use hv_setup_*() to set up hypercall arguments
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@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().
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.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 v3:
* Removed change to definition of struct hv_mmio_write_input so it remains
consistent with original Hyper-V definitions. Adjusted argument to
hv_hvcall_in_array() accordingly so that the 64 byte 'data' array is
not zero'ed. [Nuno Das Neves]
Changes in v2:
* In hv_arch_irq_unmask(), added check of 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]
drivers/pci/controller/pci-hyperv.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index d2b7e8ea710b..79de85d1d68b 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -620,7 +620,7 @@ static void hv_irq_retarget_interrupt(struct irq_data *data)
struct pci_dev *pdev;
unsigned long flags;
u32 var_size = 0;
- int cpu, nr_bank;
+ int cpu, nr_bank, batch_size;
u64 res;
dest = irq_data_get_effective_affinity_mask(data);
@@ -636,8 +636,8 @@ static void hv_irq_retarget_interrupt(struct irq_data *data)
local_irq_save(flags);
- params = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(params, 0, sizeof(*params));
+ batch_size = hv_setup_in_array(¶ms, sizeof(*params),
+ sizeof(params->int_target.vp_set.bank_contents[0]));
params->partition_id = HV_PARTITION_ID_SELF;
params->int_entry.source = HV_INTERRUPT_SOURCE_MSI;
params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
@@ -669,7 +669,7 @@ static void hv_irq_retarget_interrupt(struct irq_data *data)
nr_bank = cpumask_to_vpset(¶ms->int_target.vp_set, tmp);
free_cpumask_var(tmp);
- if (nr_bank <= 0) {
+ if (nr_bank <= 0 || nr_bank > batch_size) {
res = 1;
goto out;
}
@@ -1102,11 +1102,9 @@ static void hv_pci_read_mmio(struct device *dev, phys_addr_t gpa, int size, u32
/*
* Must be called with interrupts disabled so it is safe
- * to use the per-cpu input argument page. Use it for
- * both input and output.
+ * to use the per-cpu argument page.
*/
- in = *this_cpu_ptr(hyperv_pcpu_input_arg);
- out = *this_cpu_ptr(hyperv_pcpu_input_arg) + sizeof(*in);
+ hv_setup_inout(&in, sizeof(*in), &out, sizeof(*out));
in->gpa = gpa;
in->size = size;
@@ -1135,9 +1133,9 @@ static void hv_pci_write_mmio(struct device *dev, phys_addr_t gpa, int size, u32
/*
* Must be called with interrupts disabled so it is safe
- * to use the per-cpu input argument memory.
+ * to use the per-cpu argument page.
*/
- in = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ hv_setup_in_array(&in, offsetof(typeof(*in), data), sizeof(in->data[0]));
in->gpa = gpa;
in->size = size;
switch (size) {
--
2.25.1
^ permalink raw reply related
* [PATCH v4 6/7] Drivers: hv: Use hv_setup_*() to set up hypercall arguments for mshv code
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@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. Where feasible use batch size
returned by hv_setup_inout_array() instead of separate #define value.
Signed-off-by: Michael Kelley <mhklinux@outlook.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 v3:
* This patch is new in v3 due to rebasing on 6.15-rc1, which has new
mshv-related hypercalls.
drivers/hv/mshv_common.c | 31 +++------
drivers/hv/mshv_root_hv_call.c | 121 +++++++++++++--------------------
drivers/hv/mshv_root_main.c | 5 +-
3 files changed, 60 insertions(+), 97 deletions(-)
diff --git a/drivers/hv/mshv_common.c b/drivers/hv/mshv_common.c
index 6f227a8a5af7..94f8de66c096 100644
--- a/drivers/hv/mshv_common.c
+++ b/drivers/hv/mshv_common.c
@@ -17,12 +17,6 @@
#include "mshv.h"
-#define HV_GET_REGISTER_BATCH_SIZE \
- (HV_HYP_PAGE_SIZE / sizeof(union hv_register_value))
-#define HV_SET_REGISTER_BATCH_SIZE \
- ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_set_vp_registers)) \
- / sizeof(struct hv_register_assoc))
-
int hv_call_get_vp_registers(u32 vp_index, u64 partition_id, u16 count,
union hv_input_vtl input_vtl,
struct hv_register_assoc *registers)
@@ -30,24 +24,23 @@ int hv_call_get_vp_registers(u32 vp_index, u64 partition_id, u16 count,
struct hv_input_get_vp_registers *input_page;
union hv_register_value *output_page;
u16 completed = 0;
- unsigned long remaining = count;
+ unsigned long batch_size, remaining = count;
int rep_count, i;
u64 status = HV_STATUS_SUCCESS;
unsigned long flags;
local_irq_save(flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
+ batch_size = hv_setup_inout_array(&input_page, sizeof(*input_page),
+ sizeof(input_page->names[0]),
+ &output_page, 0, sizeof(*output_page));
input_page->partition_id = partition_id;
input_page->vp_index = vp_index;
input_page->input_vtl.as_uint8 = input_vtl.as_uint8;
- input_page->rsvd_z8 = 0;
- input_page->rsvd_z16 = 0;
while (remaining) {
- rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE);
+ rep_count = min(remaining, batch_size);
for (i = 0; i < rep_count; ++i)
input_page->names[i] = registers[i].name;
@@ -76,21 +69,19 @@ int hv_call_set_vp_registers(u32 vp_index, u64 partition_id, u16 count,
struct hv_input_set_vp_registers *input_page;
u16 completed = 0;
unsigned long remaining = count;
- int rep_count;
+ unsigned long rep_count, batch_size;
u64 status = HV_STATUS_SUCCESS;
unsigned long flags;
local_irq_save(flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
+ batch_size = hv_setup_in_array(&input_page, sizeof(*input_page),
+ sizeof(input_page->elements[0]));
input_page->partition_id = partition_id;
input_page->vp_index = vp_index;
input_page->input_vtl.as_uint8 = input_vtl.as_uint8;
- input_page->rsvd_z8 = 0;
- input_page->rsvd_z16 = 0;
while (remaining) {
- rep_count = min(remaining, HV_SET_REGISTER_BATCH_SIZE);
+ rep_count = min(remaining, batch_size);
memcpy(input_page->elements, registers,
sizeof(struct hv_register_assoc) * rep_count);
@@ -120,9 +111,7 @@ int hv_call_get_partition_property(u64 partition_id,
struct hv_output_get_partition_property *output;
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
- memset(input, 0, sizeof(*input));
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->partition_id = partition_id;
input->property_code = property_code;
status = hv_do_hypercall(HVCALL_GET_PARTITION_PROPERTY, input, output);
diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
index c9c274f29c3c..7217310b8fc3 100644
--- a/drivers/hv/mshv_root_hv_call.c
+++ b/drivers/hv/mshv_root_hv_call.c
@@ -22,22 +22,6 @@
#define HV_PAGE_COUNT_2M_ALIGNED(pg_count) (!((pg_count) & (0x200 - 1)))
#define HV_WITHDRAW_BATCH_SIZE (HV_HYP_PAGE_SIZE / sizeof(u64))
-#define HV_MAP_GPA_BATCH_SIZE \
- ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_map_gpa_pages)) \
- / sizeof(u64))
-#define HV_GET_VP_STATE_BATCH_SIZE \
- ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_get_vp_state)) \
- / sizeof(u64))
-#define HV_SET_VP_STATE_BATCH_SIZE \
- ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_set_vp_state)) \
- / sizeof(u64))
-#define HV_GET_GPA_ACCESS_STATES_BATCH_SIZE \
- ((HV_HYP_PAGE_SIZE - sizeof(union hv_gpa_page_access_state)) \
- / sizeof(union hv_gpa_page_access_state))
-#define HV_MODIFY_SPARSE_SPA_PAGE_HOST_ACCESS_MAX_PAGE_COUNT \
- ((HV_HYP_PAGE_SIZE - \
- sizeof(struct hv_input_modify_sparse_spa_page_host_access)) / \
- sizeof(u64))
int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
{
@@ -58,9 +42,7 @@ int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
while (remaining) {
local_irq_save(flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
- memset(input_page, 0, sizeof(*input_page));
+ hv_setup_in(&input_page, sizeof(*input_page));
input_page->partition_id = partition_id;
status = hv_do_rep_hypercall(HVCALL_WITHDRAW_MEMORY,
min(remaining, HV_WITHDRAW_BATCH_SIZE),
@@ -99,10 +81,7 @@ int hv_call_create_partition(u64 flags,
do {
local_irq_save(irq_flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
-
- memset(input, 0, sizeof(*input));
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->flags = flags;
input->compatibility_version = HV_COMPATIBILITY_21_H2;
@@ -206,11 +185,12 @@ static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_struct_count,
while (done < page_count) {
ulong i, completed, remain = page_count - done;
- int rep_count = min(remain, HV_MAP_GPA_BATCH_SIZE);
+ ulong rep_count, batch_size;
local_irq_save(irq_flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
+ batch_size = hv_setup_in_array(&input_page, sizeof(*input_page),
+ sizeof(input_page->source_gpa_page_list[0]));
+ rep_count = min(remain, batch_size);
input_page->target_partition_id = partition_id;
input_page->target_gpa_base = gfn + (done << large_shift);
input_page->map_flags = flags;
@@ -311,7 +291,7 @@ int hv_call_unmap_gpa_pages(u64 partition_id, u64 gfn, u64 page_count_4k,
int rep_count = min(remain, HV_UMAP_GPA_PAGES);
local_irq_save(irq_flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ hv_setup_in(&input_page, sizeof(*input_page));
input_page->target_partition_id = partition_id;
input_page->target_gpa_base = gfn + (done << large_shift);
@@ -340,7 +320,7 @@ int hv_call_get_gpa_access_states(u64 partition_id, u32 count, u64 gpa_base_pfn,
struct hv_input_get_gpa_pages_access_state *input_page;
union hv_gpa_page_access_state *output_page;
int completed = 0;
- unsigned long remaining = count;
+ unsigned long batch_size, remaining = count;
int rep_count, i;
u64 status = 0;
unsigned long flags;
@@ -348,13 +328,13 @@ int hv_call_get_gpa_access_states(u64 partition_id, u32 count, u64 gpa_base_pfn,
*written_total = 0;
while (remaining) {
local_irq_save(flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
+ batch_size = hv_setup_inout_array(&input_page, sizeof(*input_page),
+ 0, &output_page, 0, sizeof(*output_page));
input_page->partition_id = partition_id;
input_page->hv_gpa_page_number = gpa_base_pfn + *written_total;
input_page->flags = state_flags;
- rep_count = min(remaining, HV_GET_GPA_ACCESS_STATES_BATCH_SIZE);
+ rep_count = min(remaining, batch_size);
status = hv_do_rep_hypercall(HVCALL_GET_GPA_PAGES_ACCESS_STATES, rep_count,
0, input_page, output_page);
@@ -384,8 +364,7 @@ int hv_call_assert_virtual_interrupt(u64 partition_id, u32 vector,
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));
input->partition_id = partition_id;
input->vector = vector;
input->dest_addr = dest_addr;
@@ -422,21 +401,21 @@ int hv_call_get_vp_state(u32 vp_index, u64 partition_id,
u64 status;
int i;
u64 control;
- unsigned long flags;
+ unsigned long flags, batch_size;
int ret = 0;
- if (page_count > HV_GET_VP_STATE_BATCH_SIZE)
- return -EINVAL;
-
if (!page_count && !ret_output)
return -EINVAL;
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
- memset(input, 0, sizeof(*input));
- memset(output, 0, sizeof(*output));
+ batch_size = hv_setup_inout_array(&input, sizeof(*input),
+ sizeof(input->output_data_pfns[0]),
+ &output, sizeof(*output), 0);
+ if (page_count > batch_size) {
+ local_irq_restore(flags);
+ return -EINVAL;
+ }
input->partition_id = partition_id;
input->vp_index = vp_index;
@@ -478,11 +457,7 @@ int hv_call_set_vp_state(u32 vp_index, u64 partition_id,
unsigned long flags;
int ret = 0;
u16 varhead_sz;
-
- if (page_count > HV_SET_VP_STATE_BATCH_SIZE)
- return -EINVAL;
- if (sizeof(*input) + num_bytes > HV_HYP_PAGE_SIZE)
- return -EINVAL;
+ u64 batch_size;
if (num_bytes)
/* round up to 8 and divide by 8 */
@@ -494,18 +469,26 @@ int hv_call_set_vp_state(u32 vp_index, u64 partition_id,
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(input, 0, sizeof(*input));
- input->partition_id = partition_id;
- input->vp_index = vp_index;
- input->state_data = state_data;
if (num_bytes) {
+ batch_size = hv_setup_in_array(&input, sizeof(*input),
+ sizeof(input->data[0].bytes));
+ if (num_bytes > batch_size)
+ goto size_error;
+
memcpy((u8 *)input->data, bytes, num_bytes);
} else {
+ batch_size = hv_setup_in_array(&input, sizeof(*input),
+ sizeof(input->data[0].pfns));
+ if (page_count > batch_size)
+ goto size_error;
+
for (i = 0; i < page_count; i++)
input->data[i].pfns = page_to_pfn(pages[i]);
}
+ input->partition_id = partition_id;
+ input->vp_index = vp_index;
+ input->state_data = state_data;
control = (HVCALL_SET_VP_STATE) |
(varhead_sz << HV_HYPERCALL_VARHEAD_OFFSET);
@@ -524,6 +507,10 @@ int hv_call_set_vp_state(u32 vp_index, u64 partition_id,
} while (!ret);
return ret;
+
+size_error:
+ local_irq_restore(flags);
+ return -EINVAL;
}
int hv_call_map_vp_state_page(u64 partition_id, u32 vp_index, u32 type,
@@ -539,8 +526,7 @@ int hv_call_map_vp_state_page(u64 partition_id, u32 vp_index, u32 type,
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->partition_id = partition_id;
input->vp_index = vp_index;
@@ -574,9 +560,7 @@ int hv_call_unmap_vp_state_page(u64 partition_id, u32 vp_index, u32 type,
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
- memset(input, 0, sizeof(*input));
+ hv_setup_in(&input, sizeof(*input));
input->partition_id = partition_id;
input->vp_index = vp_index;
@@ -614,8 +598,7 @@ hv_call_create_port(u64 port_partition_id, union hv_port_id port_id,
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(input, 0, sizeof(*input));
+ hv_setup_in(&input, sizeof(*input));
input->port_partition_id = port_partition_id;
input->port_id = port_id;
@@ -668,8 +651,7 @@ hv_call_connect_port(u64 port_partition_id, union hv_port_id port_id,
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- memset(input, 0, sizeof(*input));
+ hv_setup_in(&input, sizeof(*input));
input->port_partition_id = port_partition_id;
input->port_id = port_id;
input->connection_partition_id = connection_partition_id;
@@ -736,10 +718,7 @@ int hv_call_map_stat_page(enum hv_stats_object_type type,
do {
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
-
- memset(input, 0, sizeof(*input));
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->type = type;
input->identity = *identity;
@@ -773,9 +752,7 @@ int hv_call_unmap_stat_page(enum hv_stats_object_type type,
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));
input->type = type;
input->identity = *identity;
@@ -808,14 +785,14 @@ int hv_call_modify_spa_host_access(u64 partition_id, struct page **pages,
}
while (done < page_count) {
- ulong i, completed, remain = page_count - done;
- int rep_count = min(remain,
- HV_MODIFY_SPARSE_SPA_PAGE_HOST_ACCESS_MAX_PAGE_COUNT);
+ ulong i, batch_size, completed, remain = page_count - done;
+ ulong rep_count;
local_irq_save(irq_flags);
- input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ batch_size = hv_setup_in_array(&input_page, sizeof(*input_page),
+ sizeof(input_page->spa_page_list[0]));
+ rep_count = min(remain, batch_size);
- memset(input_page, 0, sizeof(*input_page));
/* Only set the partition id if you are making the pages
* exclusive
*/
diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
index 72df774e410a..aca3331ad516 100644
--- a/drivers/hv/mshv_root_main.c
+++ b/drivers/hv/mshv_root_main.c
@@ -2051,11 +2051,8 @@ static int __init hv_retrieve_scheduler_type(enum hv_scheduler_type *out)
u64 status;
local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = *this_cpu_ptr(hyperv_pcpu_output_arg);
- memset(input, 0, sizeof(*input));
- memset(output, 0, sizeof(*output));
+ hv_setup_inout(&input, sizeof(*input), &output, sizeof(*output));
input->property_id = HV_SYSTEM_PROPERTY_SCHEDULER_TYPE;
status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input, output);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 7/7] Drivers: hv: Replace hyperv_pcpu_input/output_arg with hyperv_pcpu_arg
From: mhkelley58 @ 2025-07-18 4:55 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd
Cc: x86, linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@outlook.com>
All open coded uses of hyperv_pcpu_input_arg and hyperv_pcpu_ouput_arg
have been replaced by hv_setup_*() functions. So combine
hyperv_pcpu_input_arg and hyperv_pcpu_output_arg in a single
hyperv_pcpu_arg. Remove logic for managing a separate output arg. Fixup
comment references to the old variable names.
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
arch/x86/hyperv/hv_init.c | 6 ++--
drivers/hv/hv.c | 2 +-
drivers/hv/hv_common.c | 55 ++++++++++------------------------
drivers/hv/hyperv_vmbus.h | 2 +-
include/asm-generic/mshyperv.h | 6 +---
5 files changed, 22 insertions(+), 49 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b7a2877c2a92..2979d15223cf 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -453,16 +453,16 @@ void __init hyperv_init(void)
* A TDX VM with no paravisor only uses TDX GHCI rather than hv_hypercall_pg:
* when the hypercall input is a page, such a VM must pass a decrypted
* page to Hyper-V, e.g. hv_post_message() uses the per-CPU page
- * hyperv_pcpu_input_arg, which is decrypted if no paravisor is present.
+ * hyperv_pcpu_arg, which is decrypted if no paravisor is present.
*
* A TDX VM with the paravisor uses hv_hypercall_pg for most hypercalls,
* which are handled by the paravisor and the VM must use an encrypted
- * input page: in such a VM, the hyperv_pcpu_input_arg is encrypted and
+ * input page: in such a VM, the hyperv_pcpu_arg is encrypted and
* used in the hypercalls, e.g. see hv_mark_gpa_visibility() and
* hv_arch_irq_unmask(). Such a VM uses TDX GHCI for two hypercalls:
* 1. HVCALL_SIGNAL_EVENT: see vmbus_set_event() and _hv_do_fast_hypercall8().
* 2. HVCALL_POST_MESSAGE: the input page must be a decrypted page, i.e.
- * hv_post_message() in such a VM can't use the encrypted hyperv_pcpu_input_arg;
+ * hv_post_message() in such a VM can't use the encrypted hyperv_pcpu_arg;
* instead, hv_post_message() uses the post_msg_page, which is decrypted
* in such a VM and is only used in such a VM.
*/
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index ad063f535f95..3b5dfdecdfe7 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -60,7 +60,7 @@ int hv_post_message(union hv_connection_id connection_id,
/*
* A TDX VM with the paravisor must use the decrypted post_msg_page: see
* the comment in struct hv_per_cpu_context. A SNP VM with the paravisor
- * can use the encrypted hyperv_pcpu_input_arg because it copies the
+ * can use the encrypted hyperv_pcpu_arg because it copies the
* input into the GHCB page, which has been decrypted by the paravisor.
*/
if (hv_isolation_type_tdx() && ms_hyperv.paravisor_present)
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index ae56397af1ed..cbe4a954ad46 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -58,11 +58,8 @@ EXPORT_SYMBOL_GPL(hv_vp_index);
u32 hv_max_vp_index;
EXPORT_SYMBOL_GPL(hv_max_vp_index);
-void * __percpu *hyperv_pcpu_input_arg;
-EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
-
-void * __percpu *hyperv_pcpu_output_arg;
-EXPORT_SYMBOL_GPL(hyperv_pcpu_output_arg);
+void * __percpu *hyperv_pcpu_arg;
+EXPORT_SYMBOL_GPL(hyperv_pcpu_arg);
static void hv_kmsg_dump_unregister(void);
@@ -95,11 +92,8 @@ void __init hv_common_free(void)
kfree(hv_vp_index);
hv_vp_index = NULL;
- free_percpu(hyperv_pcpu_output_arg);
- hyperv_pcpu_output_arg = NULL;
-
- free_percpu(hyperv_pcpu_input_arg);
- hyperv_pcpu_input_arg = NULL;
+ free_percpu(hyperv_pcpu_arg);
+ hyperv_pcpu_arg = NULL;
free_percpu(hv_synic_eventring_tail);
hv_synic_eventring_tail = NULL;
@@ -255,11 +249,6 @@ static void hv_kmsg_dump_register(void)
}
}
-static inline bool hv_output_page_exists(void)
-{
- return hv_root_partition() || IS_ENABLED(CONFIG_HYPERV_VTL_MODE);
-}
-
void __init hv_get_partition_id(void)
{
struct hv_output_get_partition_id *output;
@@ -365,14 +354,8 @@ int __init hv_common_init(void)
* (per-CPU) hypercall input page and thus this failure is
* fatal on Hyper-V.
*/
- hyperv_pcpu_input_arg = alloc_percpu(void *);
- BUG_ON(!hyperv_pcpu_input_arg);
-
- /* Allocate the per-CPU state for output arg for root */
- if (hv_output_page_exists()) {
- hyperv_pcpu_output_arg = alloc_percpu(void *);
- BUG_ON(!hyperv_pcpu_output_arg);
- }
+ hyperv_pcpu_arg = alloc_percpu(void *);
+ BUG_ON(!hyperv_pcpu_arg);
if (hv_root_partition()) {
hv_synic_eventring_tail = alloc_percpu(u8 *);
@@ -466,33 +449,28 @@ void __init ms_hyperv_late_init(void)
int hv_common_cpu_init(unsigned int cpu)
{
- void **inputarg, **outputarg;
+ void **inputarg;
u8 **synic_eventring_tail;
u64 msr_vp_index;
gfp_t flags;
- const int pgcount = hv_output_page_exists() ? 2 : 1;
+ const int pgcount = HV_HVCALL_ARG_PAGES;
void *mem;
int ret = 0;
/* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
flags = irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL;
- inputarg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
+ inputarg = (void **)this_cpu_ptr(hyperv_pcpu_arg);
/*
- * The per-cpu memory is already allocated if this CPU was previously
- * online and then taken offline
+ * hyperv_pcpu_arg memory is already allocated if this CPU was
+ * previously online and then taken offline
*/
if (!*inputarg) {
mem = kmalloc(pgcount * HV_HYP_PAGE_SIZE, flags);
if (!mem)
return -ENOMEM;
- if (hv_output_page_exists()) {
- outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
- *outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
- }
-
if (!ms_hyperv.paravisor_present &&
(hv_isolation_type_snp() || hv_isolation_type_tdx())) {
ret = set_memory_decrypted((unsigned long)mem, pgcount);
@@ -506,13 +484,13 @@ int hv_common_cpu_init(unsigned int cpu)
/*
* In a fully enlightened TDX/SNP VM with more than 64 VPs, if
- * hyperv_pcpu_input_arg is not NULL, set_memory_decrypted() ->
+ * hyperv_pcpu_arg is not NULL, set_memory_decrypted() ->
* ... -> cpa_flush()-> ... -> __send_ipi_mask_ex() tries to
- * use hyperv_pcpu_input_arg as the hypercall input page, which
+ * use hyperv_pcpu_arg as the hypercall input page, which
* must be a decrypted page in such a VM, but the page is still
* encrypted before set_memory_decrypted() returns. Fix this by
* setting *inputarg after the above set_memory_decrypted(): if
- * hyperv_pcpu_input_arg is NULL, __send_ipi_mask_ex() returns
+ * hyperv_pcpu_arg is NULL, __send_ipi_mask_ex() returns
* HV_STATUS_INVALID_PARAMETER immediately, and the function
* hv_send_ipi_mask() falls back to orig_apic.send_IPI_mask(),
* which may be slightly slower than the hypercall, but still
@@ -544,9 +522,8 @@ int hv_common_cpu_die(unsigned int cpu)
{
u8 **synic_eventring_tail;
/*
- * The hyperv_pcpu_input_arg and hyperv_pcpu_output_arg memory
- * is not freed when the CPU goes offline as the hyperv_pcpu_input_arg
- * may be used by the Hyper-V vPCI driver in reassigning interrupts
+ * The hyperv_pcpu_arg memory is not freed when the CPU goes offline as
+ * it may be used by the Hyper-V vPCI driver in reassigning interrupts
* as part of the offlining process. The interrupt reassignment
* happens *after* the CPUHP_AP_HYPERV_ONLINE state has run and
* called this function.
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 0b450e53161e..bef8a57100ec 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -126,7 +126,7 @@ struct hv_per_cpu_context {
/*
* The page is only used in hv_post_message() for a TDX VM (with the
* paravisor) to post a messages to Hyper-V: when such a VM calls
- * HVCALL_POST_MESSAGE, it can't use the hyperv_pcpu_input_arg (which
+ * HVCALL_POST_MESSAGE, it can't use the hyperv_pcpu_arg (which
* is encrypted in such a VM) as the hypercall input page, because
* the input page for HVCALL_POST_MESSAGE must be decrypted in such a
* VM, so post_msg_page (which is decrypted in hv_synic_alloc()) is
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 040c4650f411..3ce5b94ad41e 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -67,8 +67,7 @@ extern bool hv_nested;
extern u64 hv_current_partition_id;
extern enum hv_partition_type hv_curr_partition_type;
-extern void * __percpu *hyperv_pcpu_input_arg;
-extern void * __percpu *hyperv_pcpu_output_arg;
+extern void * __percpu *hyperv_pcpu_arg;
u64 hv_do_hypercall(u64 control, void *inputaddr, void *outputaddr);
u64 hv_do_fast_hypercall8(u16 control, u64 input8);
@@ -155,9 +154,6 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
* Hypercall input and output argument setup
*/
-/* Temporary mapping to be removed at the end of the patch series */
-#define hyperv_pcpu_arg hyperv_pcpu_input_arg
-
/*
* Allocate one page that is shared between input and output args, which is
* sufficient for all current hypercalls. If a future hypercall requires
--
2.25.1
^ permalink raw reply related
* [PATCH RESEND] netvsc: transfer lower device max tso size
From: Cindy Lu @ 2025-07-18 6:17 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Michael Kelley, Shradha Gupta, Kees Cook,
Jason Wang, Stanislav Fomichev, Kuniyuki Iwashima,
Alexander Lobakin, Guillaume Nault, Joe Damato, Ahmed Zaki,
open list:Hyper-V/Azure CORE AND DRIVERS,
open list:NETWORKING DRIVERS, open list, lulu
From: Jason Wang <jasowang@redhat.com>
When netvsc is accelerated by the lower device, we can advertise the
lower device max tso size in order to get better performance.
One example is that when 802.3ad encap is enabled by netvsc, it has a
lower max tso size than 64K. This will lead to software segmentation
of forwarding GSO packet (e.g the one from VM/tap).
This patch help to recover the performance.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Tested-by: Cindy Lu <lulu@redhat.com>
---
drivers/net/hyperv/netvsc_drv.c | 2 +-
include/linux/netdevice.h | 4 ++++
net/core/dev.c | 18 ++++++++++++++++++
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c41a025c66f0..7af4aa4f4abe 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2440,7 +2440,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
* switched over to the VF
*/
if (vf_is_up)
- netif_set_tso_max_size(ndev, vf_netdev->tso_max_size);
+ netif_stacked_transfer_tso_max_size(vf_netdev, ndev);
else
netif_set_tso_max_size(ndev, netvsc_dev->netvsc_gso_max_size);
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index adb14db25798..c695a3ffecd8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5275,6 +5275,9 @@ void netdev_change_features(struct net_device *dev);
void netif_stacked_transfer_operstate(const struct net_device *rootdev,
struct net_device *dev);
+void netif_stacked_transfer_tso_max_size(const struct net_device *rootdev,
+ struct net_device *dev);
+
netdev_features_t passthru_features_check(struct sk_buff *skb,
struct net_device *dev,
netdev_features_t features);
@@ -5326,6 +5329,7 @@ static inline bool netif_needs_gso(struct sk_buff *skb,
}
void netif_set_tso_max_size(struct net_device *dev, unsigned int size);
+
void netif_set_tso_max_segs(struct net_device *dev, unsigned int segs);
void netif_inherit_tso_max(struct net_device *to,
const struct net_device *from);
diff --git a/net/core/dev.c b/net/core/dev.c
index be97c440ecd5..3bec4284adff 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3306,6 +3306,24 @@ void netif_set_tso_max_size(struct net_device *dev, unsigned int size)
}
EXPORT_SYMBOL(netif_set_tso_max_size);
+/**
+ * netif_stacked_transfer_tso_max_size - transfer tso max size
+ * @rootdev: the root or lower level device to transfer tso max size from
+ * @dev: the device to transfer operstate to
+ *
+ * Transfer tso max size from root to device. This is normally
+ * called when a stacking relationship exists between the root
+ * device and the device(a leaf device).
+ */
+void netif_stacked_transfer_tso_max_size(const struct net_device *rootdev,
+ struct net_device *dev)
+{
+ dev->tso_max_size = rootdev->tso_max_size;
+ netif_set_gso_max_size(dev, READ_ONCE(rootdev->gso_max_size));
+ netif_set_gso_ipv4_max_size(dev, READ_ONCE(rootdev->gso_ipv4_max_size));
+}
+EXPORT_SYMBOL(netif_stacked_transfer_tso_max_size);
+
/**
* netif_set_tso_max_segs() - set the max number of segs supported for TSO
* @dev: netdev to update
--
2.45.0
^ permalink raw reply related
* [PATCH v2] netvsc: transfer lower device max tso size
From: Cindy Lu @ 2025-07-18 8:28 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Michael Kelley, Shradha Gupta, Kees Cook,
Saurabh Sengar, Stanislav Fomichev, Kuniyuki Iwashima,
Alexander Lobakin, Guillaume Nault, Joe Damato, Ahmed Zaki,
open list:Hyper-V/Azure CORE AND DRIVERS,
open list:NETWORKING DRIVERS, open list, lulu, jasowang
From: Jason Wang <jasowang@redhat.com>
When netvsc is accelerated by the lower device, we can advertise the
lower device max tso size in order to get better performance.
One example is that when 802.3ad encap is enabled by netvsc, it has a
lower max tso size than 64K. This will lead to software segmentation
of forwarding GSO packet (e.g the one from VM/tap).
This patch help to recover the performance.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Tested-by: Cindy Lu <lulu@redhat.com>
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/net/hyperv/netvsc_drv.c | 2 +-
include/linux/netdevice.h | 4 ++++
net/core/dev.c | 18 ++++++++++++++++++
3 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c41a025c66f0..7af4aa4f4abe 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2440,7 +2440,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
* switched over to the VF
*/
if (vf_is_up)
- netif_set_tso_max_size(ndev, vf_netdev->tso_max_size);
+ netif_stacked_transfer_tso_max_size(vf_netdev, ndev);
else
netif_set_tso_max_size(ndev, netvsc_dev->netvsc_gso_max_size);
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index adb14db25798..c695a3ffecd8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5275,6 +5275,9 @@ void netdev_change_features(struct net_device *dev);
void netif_stacked_transfer_operstate(const struct net_device *rootdev,
struct net_device *dev);
+void netif_stacked_transfer_tso_max_size(const struct net_device *rootdev,
+ struct net_device *dev);
+
netdev_features_t passthru_features_check(struct sk_buff *skb,
struct net_device *dev,
netdev_features_t features);
@@ -5326,6 +5329,7 @@ static inline bool netif_needs_gso(struct sk_buff *skb,
}
void netif_set_tso_max_size(struct net_device *dev, unsigned int size);
+
void netif_set_tso_max_segs(struct net_device *dev, unsigned int segs);
void netif_inherit_tso_max(struct net_device *to,
const struct net_device *from);
diff --git a/net/core/dev.c b/net/core/dev.c
index be97c440ecd5..3bec4284adff 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3306,6 +3306,24 @@ void netif_set_tso_max_size(struct net_device *dev, unsigned int size)
}
EXPORT_SYMBOL(netif_set_tso_max_size);
+/**
+ * netif_stacked_transfer_tso_max_size - transfer tso max size
+ * @rootdev: the root or lower level device to transfer tso max size from
+ * @dev: the device to transfer operstate to
+ *
+ * Transfer tso max size from root to device. This is normally
+ * called when a stacking relationship exists between the root
+ * device and the device(a leaf device).
+ */
+void netif_stacked_transfer_tso_max_size(const struct net_device *rootdev,
+ struct net_device *dev)
+{
+ dev->tso_max_size = rootdev->tso_max_size;
+ netif_set_gso_max_size(dev, READ_ONCE(rootdev->gso_max_size));
+ netif_set_gso_ipv4_max_size(dev, READ_ONCE(rootdev->gso_ipv4_max_size));
+}
+EXPORT_SYMBOL(netif_stacked_transfer_tso_max_size);
+
/**
* netif_set_tso_max_segs() - set the max number of segs supported for TSO
* @dev: netdev to update
--
2.45.0
^ permalink raw reply related
* RE: [PATCH v5 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Michael Kelley @ 2025-07-18 15:07 UTC (permalink / raw)
To: Naman Jain, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui
Cc: Roman Kisel, Anirudh Rayabharam, Saurabh Sengar,
Stanislav Kinsburskii, Nuno Das Neves, ALOK TIWARI,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
In-Reply-To: <42bc5294-219f-4c26-ad05-740f6190aff3@linux.microsoft.com>
From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, July 17, 2025 9:36 PM
>
> On 7/9/2025 10:49 PM, Michael Kelley wrote:
> > From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, June 11, 2025 12:27 AM
[snip]
>
> > Separately, "allow_bitmap" size is 64K bytes, or 512K bits. Is that the
> > correct size? From looking at mshv_vtl_hvcall_is_allowed(), I think this
> > bitmap is indexed by the HV call code, which is a 16 bit value. So you
> > only need 64K bits, and the size is too big by a factor of 8. In any case,
> > it seems like the size should not be expressed in terms of PAGE_SIZE.
>
> There are HVcall codes which are of type u16. So max(HVcall code) =
> 0xffff.
>
> For every HVcall that needs to be allowed, we are saving HVcall code
> info in a bitmap in below fashion:
> if x = HVCall code and bitmap is an array of u64, of size
> ((0xffff/64=1023) + 1)
>
> bitmap[x / 64] = (u64)1 << (x%64);
>
> Later on in mshv_vtl_hvcall_is_allowed(), we calculate the array index
> by dividing it by 64, and then see if call_code/64 bit is set.
I didn't add comments in mshv_vtl_hvcall_is_allowed(), but that code
can be simplified by recognizing that the Linux kernel bitmap utilities
can operate on bitmaps that are much larger than just 64 bits. Let's
assume that the allow_bitmap field in struct mshv_vtl_hvcall_fds has
64K bits, regardless of whether it is declared as an array of u64,
an array of u16, or an array of u8. Then mshv_vtl_hvcall_is_allowed()
can be implemented as a single line:
return test_bit(call_code, fd->allow_bitmap);
There's no need to figure out which array element contains the bit,
or to construct a mask to select that particular bit in the array element.
And since call_code is a u16, test_bit won't access outside the allocated
64K bits.
>
> Coming to size of allow_bitmap[], it is independent of PAGE_SIZE, and
> can be safely initialized to 1024 (reducing by a factor of 8).
> bitmap_size's maximum value is going to be 1024 in current
> implementation, picking u64 was not mandatory, u16 will also work. Also,
> item_index is also u16, so I should make bitmap_size as u16.
The key question for me is whether bitmap_size describes the number
of bits in allow_bitmap, or whether it describes the number of array
elements in the declared allow_bitmap array. It's more typical to
describe a bitmap size as the number of bits. Then the value is
independent of the array element size, as the array element size
usually doesn't really matter anyway if using the Linux kernel's
bitmap utilities. The array element size only matters in allocating
the correct amount of space is for whatever number of bits are
needed in the bitmap.
[snip]
> >> +
> >> + event_flags = (union hv_synic_event_flags *)per_cpu->synic_event_page +
> >> + VTL2_VMBUS_SINT_INDEX;
> >> + for (i = 0; i < HV_EVENT_FLAGS_LONG_COUNT; i++) {
> >> + if (READ_ONCE(event_flags->flags[i])) {
> >> + word = xchg(&event_flags->flags[i], 0);
> >> + for_each_set_bit(j, &word, BITS_PER_LONG) {
> >
> > Is there a reason for the complexity in finding and resetting bits that are
> > set in the sync_event_page? See the code in vmbus_chan_sched() that I
> > think is doing the same thing, but with simpler code.
>
> I am sorry, but I am not sure how this can be written similar to
> vmbus_chan_sched(). We don't have eventfd signaling mechanism there.
> Can you please share some more info/code snippet of what you were
> suggesting?
See below.
>
>
> >
> >> + rcu_read_lock();
> >> + eventfd = READ_ONCE(flag_eventfds[i * BITS_PER_LONG + j]);
> >> + if (eventfd)
> >> + eventfd_signal(eventfd);
> >> + rcu_read_unlock();
> >> + }
> >> + }
> >> + }
Here's what I would suggest. As with the hvcall allow_bitmap, this uses
the Linux kernel bitmap utilities' ability to operate on large bitmaps, instead
of going through each ulong in the array, and then going through each bit
in the ulong.
event_flags = (union hv_synic_event_flags *)per_cpu->synic_event_page + VTL2_VMBUS_SINT_INDEX;
for_each_set_bit(i, event_flags->flags, HV_EVENT_FLAGS_COUNT) {
if (!sync_test_and_clear_bit(i, event_flags->flags))
continue;
rcu_read_lock();
eventfd = READ_ONCE(flag_eventfds[i]);
if (eventfd)
eventfd_signal(eventfd);
rcu_read_unlock();
}
I haven't even compile tested the above, but hopefully you get the
idea and can fix any stupid mistakes. Note that HV_EVENT_FLAGS_COUNT
is a bit count, not a count of ulong's. And with the above code, you don't
need to add a definition of HV_EVENT_FLAGS_LONG_COUNT.
[snip]
> >> + pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
> >> + if (!pgmap)
> >> + return -ENOMEM;
> >> +
> >> + pgmap->ranges[0].start = PFN_PHYS(vtl0_mem.start_pfn);
> >> + pgmap->ranges[0].end = PFN_PHYS(vtl0_mem.last_pfn) - 1;
> >
> > Perhaps this should be
> >
> > pgmap->ranges[0].end = PFN_PHYS(vtl0_mem.last_pfn + 1) - 1
> >
> > otherwise the last page won't be included in the range. Or is excluding the
> > last page intentional?
>
> Excluding the last page is intentional. Hence there is a check for this
> as well:
> if (vtl0_mem.last_pfn <= vtl0_mem.start_pfn) {
>
OK, this test requires that at least 2 PFNs be provided, because the
last one will be excluded.
I'd suggest adding a comment that the last page is intentionally
excluded, and why it is excluded. Somebody in future looking at this
code will appreciate the explanation. :-)
[snip]
> >
> >> +
> >> + if (!cpu_online(input.cpu))
> >> + return -EINVAL;
> >
> > Having tested that the target CPU is online, does anything ensure that the
> > CPU stays online during the completion of this function? Usually the
> > cpus_read_lock() needs to be held to ensure that an online CPU stays
> > online for the duration of an operation.
>
> Added cpus_read_lock() block around per_cpu_ptr operation. In general,
> CPUs are never hotplugged in kernel from our Usecase POV. I have omitted
> adding these locks at other places for now. Please let me know your
> thoughts on this, in case you feel we need to have it.
>
My understanding of VTL2 behavior is limited, so let me ask some clarifying
questions. If a vCPU is running in VTL0, then presumably that vCPU is also
running in VTL2. If that vCPU is then taken offline in VTL0, does it stay
online in VTL2? And then if the vCPU is brought back online in VTL0,
nothing changes in VTL2, correct?
If that is the correct understanding, and vCPUs never go offline in VTL2,
it would be more robust to enforce that. For example, in hv_vtl_setup_synic()
where cpuhp_setup_state() is called, the teardown argument is currently
NULL. You could provide a teardown function that just returns an error.
Then any attempts to take a vCPU offline in VTL2 would fail, and the vCPU
would stay online. However, some additional logic might be needed to
ensure that normal shutdown and the panic case work correctly -- I'm not
sure what VTL2 needs to do for these scenarios.
All that said, if you can be sure that vCPUs don't go offline in VTL2,
I would be OK with not adding the cpus_read_lock(). Perhaps a comment
would be helpful in the places where you are not using cpus_read_lock()
for this reason, assuming there is a reasonable number of such places.
[snip]
> >> +
> >> +struct mshv_vtl_hvcall_setup {
> >> + __u64 bitmap_size;
> >
> > What are the units of "bitmap_size"? Bits? Bytes? u64?
>
> It would be length of bitmap array.
To me "length of bitmap array" is still ambiguous. Is it the
number of elements in the declared array field? Per my
earlier comments, I think the number of bits in the bitmap
would be more typical.
>
> >
> >> + __u64 allow_bitmap_ptr; /* pointer to __u64 */
> >> +};
Michael
^ permalink raw reply
* Re: [PATCH v4 0/7] hyperv: Introduce new way to manage hypercall args
From: Easwar Hariharan @ 2025-07-18 16:33 UTC (permalink / raw)
To: mhklinux
Cc: kys, haiyangz, wei.liu, decui, tglx, mingo, bp, dave.hansen, hpa,
lpieralisi, kw, mani, robh, bhelgaas, arnd, eahariha, x86,
linux-hyperv, linux-kernel, linux-pci, linux-arch
In-Reply-To: <20250718045545.517620-1-mhklinux@outlook.com>
On 7/17/2025 9:55 PM, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
>
> This patch set introduces a new way to manage the use of the per-vCPU
> memory that is usually the input and output arguments to Hyper-V
> hypercalls. Current code allocates the "hyperv_pcpu_input_arg", and in
> some configurations, the "hyperv_pcpu_output_arg". Each is a 4 KiB
> page of memory allocated per-vCPU. A hypercall call site disables
> interrupts, then uses this memory to set up the input parameters for
> the hypercall, read the output results after hypercall execution, and
> re-enable interrupts. The open coding of these steps has led to
> inconsistencies, and in some cases, violation of the generic
> requirements for the hypercall input and output as described in the
> Hyper-V Top Level Functional Spec (TLFS)[1]. This patch set introduces
> a new family of inline functions to replace the open coding. The new
> functions encapsulate key aspects of the use of per-vCPU memory for
> hypercall input and output, and ensure that the TLFS requirements are
> met (max size of 1 page each for input and output, no overlap of input
> and output, aligned to 8 bytes, etc.).
>
> With this change, hypercall call sites no longer directly access
> "hyperv_pcpu_input_arg" and "hyperv_pcpu_output_arg". Instead, one of
> a family of new functions provides the per-vCPU memory that a hypercall
> call site uses to set up hypercall input and output areas.
> Conceptually, there is no longer a difference between the "per-vCPU
> input page" and "per-vCPU output page". Only a single per-vCPU page is
> allocated, and it is used to provide both hypercall input and output.
> All current hypercalls can fit their input and output within that single
> page, though the new code allows easy changing to two pages should a
> future hypercall require a full page for each of the input and output.
>
> The new functions always zero the fixed-size portion of the hypercall
> input area (but not any array portion -- see below) so that
> uninitialized memory isn't inadvertently passed to the hypercall.
> Current open-coded hypercall call sites are inconsistent on this point,
> and use of the new functions addresses that inconsistency. The output
> area is not zero'ed by the new code as it is Hyper-V's responsibility
> to provide legal output.
>
> When the input or output (or both) contain an array, the new code
> calculates and returns how many array entries fit within the per-vCPU
> memory page, which is effectively the "batch size" for the hypercall
> processing multiple entries. This batch size can then be used in the
> hypercall control word to specify the repetition count. This
> calculation of the batch size replaces current open coding of the
> batch size, which is prone to errors. Note that the array portion of
> the input area is *not* zero'ed. The arrays are almost always 64-bit
> GPAs or something similar, and zero'ing that much memory seems
> wasteful at runtime when it will all be overwritten. The hypercall
> call site is responsible for ensuring that no part of the array is
> left uninitialized (just as with current code).
>
> The new family of functions is realized as a single inline function
> that handles the most complex case, which is a hypercall with input
> and output, both of which contain arrays. Simpler cases are mapped to
> this most complex case with #define wrappers that provide zero or NULL
> for some arguments. Several of the arguments to this new function
> must be compile-time constants generated by "sizeof()" expressions.
> As such, most of the code in the new function is evaluated by the
> compiler, with the result that the runtime code paths are no longer
> than with the current open coding. An exception is the new code
> generated to zero the fixed-size portion of the input area in cases
> where it was not previously done.
>
> Use of the new function typically (but not always) saves a few lines
> of code at each hypercall call site. This is traded off against the
> lines of code added for the new functions. With code currently
> upstream, the net is an add of about 20 lines of code and comments.
>
> A couple hypercall call sites have requirements that are not 100%
> handled by the new function. These still require some manual open-
> coded adjustment or open-coded batch size calculations -- see the
> individual patches in this series. Suggestions on how to do better
> are welcome.
>
> The patches in the series do the following:
>
> Patch 1: Introduce the new family of functions for assigning hypercall
> input and output arguments.
>
> Patch 2 to 6: Change existing hypercall call sites to use one of the new
> functions. In some cases, tweaks to the hypercall argument data
> structures are necessary, but these tweaks are making the data
> structures more consistent with the overall pattern. These
> 5 patches are independent of each other, and can go in any
> order. The breakup into 5 patches is for ease of review.
>
> Patch 7: Update the name of the variable used to hold the per-vCPU memory
> used for hypercall arguments. Remove code for managing the
> per-vCPU output page.
>
> The new code compiles and runs successfully on x86 and arm64. However,
> basic smoke tests cover only a limited number of hypercall call sites
> that have been modified. I don't have the hardware or Hyper-V
> configurations needed to test running in the Hyper-V root partition
> or running in a VTL other than VTL 0. The related hypercall call sites
> still need to be tested to make sure I didn't break anything. Hopefully
> someone with the necessary configurations and Hyper-V versions can
> help with that testing.
>
> For gcc 9.4.0, I've looked at the generated code for a couple of
> hypercall call sites on both x86 and arm64 to ensure that it boils
> down to the equivalent of the current open coding. I have not looked
> at the generated code for later gcc versions or for Clang/LLVM, but
> there's no reason to expect something worse as the code isn't doing
> anything tricky.
>
> This patch set is built against linux-next20250716.
>
> [1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/tlfs
>
> Michael Kelley (7):
> Drivers: hv: Introduce hv_setup_*() functions for hypercall arguments
> x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 1
> x86/hyperv: Use hv_setup_*() to set up hypercall arguments -- part 2
> Drivers: hv: Use hv_setup_*() to set up hypercall arguments
> PCI: hv: Use hv_setup_*() to set up hypercall arguments
> Drivers: hv: Use hv_setup_*() to set up hypercall arguments for mshv
> code
> Drivers: hv: Replace hyperv_pcpu_input/output_arg with hyperv_pcpu_arg
>
> arch/x86/hyperv/hv_apic.c | 10 +--
> arch/x86/hyperv/hv_init.c | 12 ++-
> arch/x86/hyperv/hv_vtl.c | 3 +-
> arch/x86/hyperv/irqdomain.c | 17 ++--
> arch/x86/hyperv/ivm.c | 18 ++---
> arch/x86/hyperv/mmu.c | 19 ++---
> arch/x86/hyperv/nested.c | 14 ++--
> drivers/hv/hv.c | 6 +-
> drivers/hv/hv_balloon.c | 8 +-
> drivers/hv/hv_common.c | 64 +++++----------
> drivers/hv/hv_proc.c | 23 +++---
> drivers/hv/hyperv_vmbus.h | 2 +-
> drivers/hv/mshv_common.c | 31 +++----
> drivers/hv/mshv_root_hv_call.c | 121 +++++++++++-----------------
> drivers/hv/mshv_root_main.c | 5 +-
> drivers/pci/controller/pci-hyperv.c | 18 ++---
> include/asm-generic/mshyperv.h | 106 +++++++++++++++++++++++-
> include/hyperv/hvgdk_mini.h | 4 +-
> 18 files changed, 251 insertions(+), 230 deletions(-)
>
Thank you for spinning this version!
For the series,
Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>
^ permalink raw reply
* RE: [PATCH v4 0/7] hyperv: Introduce new way to manage hypercall args
From: Michael Kelley @ 2025-07-18 17:13 UTC (permalink / raw)
To: Easwar Hariharan
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, dave.hansen@linux.intel.com, hpa@zytor.com,
lpieralisi@kernel.org, kw@linux.com, mani@kernel.org,
robh@kernel.org, bhelgaas@google.com, arnd@arndb.de,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-arch@vger.kernel.org
In-Reply-To: <c5d4d351-a7ff-4762-8bb3-61554d4f9731@linux.microsoft.com>
From: Easwar Hariharan <eahariha@linux.microsoft.com> Sent: Friday, July 18, 2025 9:33 AM
>
> On 7/17/2025 9:55 PM, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mhklinux@outlook.com>
> >
[snip]
> >
> > The new code compiles and runs successfully on x86 and arm64. However,
> > basic smoke tests cover only a limited number of hypercall call sites
> > that have been modified. I don't have the hardware or Hyper-V
> > configurations needed to test running in the Hyper-V root partition
> > or running in a VTL other than VTL 0. The related hypercall call sites
> > still need to be tested to make sure I didn't break anything. Hopefully
> > someone with the necessary configurations and Hyper-V versions can
> > help with that testing.
Easwar --
Thanks for reviewing.
Any chance you (or someone else) could do a quick smoke test of this
patch set when running in the Hyper-V root partition, and separately,
when running in VTL2? Some hypercall call sites are modified that
don't get called in normal VTL0 execution. It just needs a quick
verification that nothing is obviously broken for the root partition and
VTL2 cases.
Michael
> >
> > This patch set is built against linux-next20250716.
> >
[snip]
>
> Thank you for spinning this version!
>
> For the series,
>
> Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] x86/hyperv: Switch to msi_create_parent_irq_domain()
From: Thomas Gleixner @ 2025-07-18 19:08 UTC (permalink / raw)
To: Nuno Das Neves, Wei Liu, Nam Cao
Cc: K . Y . Srinivasan, Marc Zyngier, Haiyang Zhang, Dexuan Cui,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin,
linux-hyperv, linux-kernel
In-Reply-To: <c145aebb-4deb-4f1e-95aa-2ba13bf0c453@linux.microsoft.com>
On Tue, Jul 15 2025 at 12:09, Nuno Das Neves wrote:
> On 7/9/2025 4:40 PM, Wei Liu wrote:
>> CC Nuno who is also touching on this file.
>>
>> Nam, thanks for this patch.
>>
>> Nuno's patches fix a few bugs with the current implementation. I expect
>> those patches to have contextual conflicts with this patch. I would like
>> to commit Nuno's patches first since those are bug fixes, and then
>> circle back to this one.
>>
>> Nuno, can you please review this patch?
>>
>
> I don't think I can give a Reviewed-by since I don't have the context
> for these changes.
The cover letter provides the information along with a link for deeper
information:
https://lore.kernel.org/all/cover.1751277765.git.namcao@linutronix.de
> It doesn't look like this conflicts with the changes in my patches.
It might not look like it, but reality tells a different story:
Applying patch _PATCH_v2_2_2_x86_hyperv_Switch_to_msi_create_parent_irq_domain_.patch
patching file arch/x86/hyperv/irqdomain.c
Hunk #1 FAILED at 10.
Hunk #2 FAILED at 276.
2 out of 2 hunks FAILED -- rejects in file arch/x86/hyperv/irqdomain.c
patching file drivers/hv/Kconfig
Hunk #1 FAILED at 10.
1 out of 1 hunk FAILED -- rejects in file drivers/hv/Kconfig
Nam, can you please redo the patch against the hyperv tree?
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms
From: dan.j.williams @ 2025-07-18 19:17 UTC (permalink / raw)
To: Michael Kelley, dan.j.williams@intel.com, bhelgaas@google.com
Cc: linux-pci@vger.kernel.org, lukas@wunner.de,
linux-kernel@vger.kernel.org, Jonathan.Cameron@huawei.com,
Suzuki K Poulose, Lorenzo Pieralisi, Rob Herring,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
open list:Hyper-V/Azure CORE AND DRIVERS
In-Reply-To: <SN6PR02MB4157A7F41F299608E605E0C5D450A@SN6PR02MB4157.namprd02.prod.outlook.com>
Michael Kelley wrote:
[..]
> > Here is the replacement fixup that I will fold in if it looks good to
> > you:
> >
> > -- 8< --
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index cfe9806bdbe4..f1079a438bff 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> > {
> > struct pci_host_bridge *bridge;
> > struct hv_pcibus_device *hbus;
> > - u16 dom_req, dom;
> > + int ret, dom;
> > + u16 dom_req;
> > char *name;
> > - int ret;
> >
> > bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> > if (!bridge)
> > @@ -3673,8 +3673,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > * collisions) in the same VM.
> > */
> > dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> > - dom = pci_bus_find_emul_domain_nr(dom_req);
> > -
>
> As an additional paragraph the larger comment block above, let's include a
> massaged version of the comment associated with HVPCI_DOM_INVALID.
> Perhaps:
>
> *
> * Because Gen1 VMs use domain 0, don't allow picking domain 0 here, even
> * if bytes 4 and 5 of the instance GUID are both zero.
> */
That looks good to me.
>
> > + dom = pci_bus_find_emul_domain_nr(dom_req, 1, U16_MAX);
> > if (dom < 0) {
> > dev_err(&hdev->device,
> > "Unable to use dom# 0x%x or other numbers", dom_req);
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index f60244ff9ef8..30935fe85af9 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -881,7 +881,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
> >
> > sd->vmd_dev = vmd->dev;
> > - sd->domain = pci_bus_find_emul_domain_nr(PCI_DOMAIN_NR_NOT_SET);
> > +
> > + /*
> > + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> > + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> > + * which the lower 16 bits are the PCI Segment Group (domain) number.
> > + * Other bits are currently reserved.
> > + */
> > + sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
> > if (sd->domain < 0)
> > return sd->domain;
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 833ebf2d5213..de42e53f07d0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6695,34 +6695,15 @@ static void pci_no_domains(void)
> > #ifdef CONFIG_PCI_DOMAINS
> > static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> >
> > -/*
> > - * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > - * fallback to the first free domain number above the last ACPI segment number.
> > - * Caller may have a specific domain number in mind, in which case try to
> > - * reserve it.
> > - *
> > - * Note that this allocation is freed by pci_release_host_bridge_dev().
> > +/**
> > + * pci_bus_find_emul_domain_nr() - allocate a PCI domain number per constraints
> > + * @hint: desired domain, 0 if any id in the range of @min to @max is acceptable
> > + * @min: minimum allowable domain
> > + * @max: maximum allowable domain, no ids higher than INT_MAX will be returned
> > */
> > -int pci_bus_find_emul_domain_nr(int hint)
> > +u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
>
> Shouldn't the return type here still be "int"? ida_alloc_range() can return a negative
> errno if it fails. And the call sites in hv_pci_probe() and vmd_enable_domain()
> store the return value into an "int".
Yup, good catch.
> Other than that, and my suggested added comment, this looks good.
Thanks!
^ permalink raw reply
* [PATCH v3 0/1] x86/hyperv: MSI parent domain conversion
From: Nam Cao @ 2025-07-18 19:57 UTC (permalink / raw)
To: K . Y . Srinivasan, Nuno Das Neves, Marc Zyngier, Haiyang Zhang,
Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv,
linux-kernel
Cc: Nam Cao
The initial implementation of PCI/MSI interrupt domains in the hierarchical
interrupt domain model used a shortcut by providing a global PCI/MSI
domain.
This works because the PCI/MSI[X] hardware is standardized and uniform, but
it violates the basic design principle of hierarchical interrupt domains:
Each hardware block involved in the interrupt delivery chain should have a
separate interrupt domain.
For PCI/MSI[X], the interrupt controller is per PCI device and not a global
made-up entity.
Unsurprisingly, the shortcut turned out to have downsides as it does not
allow dynamic allocation of interrupt vectors after initialization and it
prevents supporting IMS on PCI. For further details, see:
https://lore.kernel.org/lkml/20221111120501.026511281@linutronix.de/
The solution is implementing per device MSI domains, this means the
entities which provide global PCI/MSI domain so far have to implement MSI
parent domain functionality instead.
This series converts the x86 hyperv driver to implement MSI parent domain.
Changes in v3:
- Drop the merged patch
- Rebase onto hyperv-fixes
arch/x86/hyperv/irqdomain.c | 111 ++++++++++++++++++++++++------------
drivers/hv/Kconfig | 1 +
2 files changed, 77 insertions(+), 35 deletions(-)
--
2.49.0
^ permalink raw reply
* [PATCH v3 1/1] x86/hyperv: Switch to msi_create_parent_irq_domain()
From: Nam Cao @ 2025-07-18 19:57 UTC (permalink / raw)
To: K . Y . Srinivasan, Nuno Das Neves, Marc Zyngier, Haiyang Zhang,
Wei Liu, Dexuan Cui, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, linux-hyperv,
linux-kernel
Cc: Nam Cao
In-Reply-To: <cover.1752868165.git.namcao@linutronix.de>
Move away from the legacy MSI domain setup, switch to use
msi_create_parent_irq_domain().
While doing the conversion, I noticed that hv_irq_compose_msi_msg() is
doing more than it is supposed to (composing message content). The
interrupt allocation bits should be moved into hv_msi_domain_alloc().
However, I have no hardware to test this change, therefore I leave a TODO
note.
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
arch/x86/hyperv/irqdomain.c | 111 ++++++++++++++++++++++++------------
drivers/hv/Kconfig | 1 +
2 files changed, 77 insertions(+), 35 deletions(-)
diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
index 090f5ac9f492..c3ba12b1bc07 100644
--- a/arch/x86/hyperv/irqdomain.c
+++ b/arch/x86/hyperv/irqdomain.c
@@ -11,6 +11,7 @@
#include <linux/pci.h>
#include <linux/irq.h>
#include <linux/export.h>
+#include <linux/irqchip/irq-msi-lib.h>
#include <asm/mshyperv.h>
static int hv_map_interrupt(union hv_device_id device_id, bool level,
@@ -289,59 +290,99 @@ static void hv_teardown_msi_irq(struct pci_dev *dev, struct irq_data *irqd)
(void)hv_unmap_msi_interrupt(dev, &old_entry);
}
-static void hv_msi_free_irq(struct irq_domain *domain,
- struct msi_domain_info *info, unsigned int virq)
-{
- struct irq_data *irqd = irq_get_irq_data(virq);
- struct msi_desc *desc;
-
- if (!irqd)
- return;
-
- desc = irq_data_get_msi_desc(irqd);
- if (!desc || !desc->irq || WARN_ON_ONCE(!dev_is_pci(desc->dev)))
- return;
-
- hv_teardown_msi_irq(to_pci_dev(desc->dev), irqd);
-}
-
/*
* IRQ Chip for MSI PCI/PCI-X/PCI-Express Devices,
* which implement the MSI or MSI-X Capability Structure.
*/
static struct irq_chip hv_pci_msi_controller = {
.name = "HV-PCI-MSI",
- .irq_unmask = pci_msi_unmask_irq,
- .irq_mask = pci_msi_mask_irq,
.irq_ack = irq_chip_ack_parent,
- .irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_compose_msi_msg = hv_irq_compose_msi_msg,
- .irq_set_affinity = msi_domain_set_affinity,
- .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MOVE_DEFERRED,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
};
-static struct msi_domain_ops pci_msi_domain_ops = {
- .msi_free = hv_msi_free_irq,
- .msi_prepare = pci_msi_prepare,
+static bool hv_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
+ struct irq_domain *real_parent, struct msi_domain_info *info)
+{
+ struct irq_chip *chip = info->chip;
+
+ if (!msi_lib_init_dev_msi_info(dev, domain, real_parent, info))
+ return false;
+
+ chip->flags |= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MOVE_DEFERRED;
+
+ info->ops->msi_prepare = pci_msi_prepare;
+
+ return true;
+}
+
+#define HV_MSI_FLAGS_SUPPORTED (MSI_GENERIC_FLAGS_MASK | MSI_FLAG_PCI_MSIX)
+#define HV_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS)
+
+static struct msi_parent_ops hv_msi_parent_ops = {
+ .supported_flags = HV_MSI_FLAGS_SUPPORTED,
+ .required_flags = HV_MSI_FLAGS_REQUIRED,
+ .bus_select_token = DOMAIN_BUS_NEXUS,
+ .bus_select_mask = MATCH_PCI_MSI,
+ .chip_flags = MSI_CHIP_FLAG_SET_ACK,
+ .prefix = "HV-",
+ .init_dev_msi_info = hv_init_dev_msi_info,
};
-static struct msi_domain_info hv_pci_msi_domain_info = {
- .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
- MSI_FLAG_PCI_MSIX,
- .ops = &pci_msi_domain_ops,
- .chip = &hv_pci_msi_controller,
- .handler = handle_edge_irq,
- .handler_name = "edge",
+static int hv_msi_domain_alloc(struct irq_domain *d, unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ /*
+ * TODO: The allocation bits of hv_irq_compose_msi_msg(), i.e. everything except
+ * entry_to_msi_msg() should be in here.
+ */
+
+ int ret;
+
+ ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg);
+ if (ret)
+ return ret;
+
+ for (int i = 0; i < nr_irqs; ++i) {
+ irq_domain_set_info(d, virq + i, 0, &hv_pci_msi_controller, NULL,
+ handle_edge_irq, NULL, "edge");
+ }
+ return 0;
+}
+
+static void hv_msi_domain_free(struct irq_domain *d, unsigned int virq, unsigned int nr_irqs)
+{
+ for (int i = 0; i < nr_irqs; ++i) {
+ struct irq_data *irqd = irq_domain_get_irq_data(d, virq);
+ struct msi_desc *desc;
+
+ desc = irq_data_get_msi_desc(irqd);
+ if (!desc || !desc->irq || WARN_ON_ONCE(!dev_is_pci(desc->dev)))
+ continue;
+
+ hv_teardown_msi_irq(to_pci_dev(desc->dev), irqd);
+ }
+ irq_domain_free_irqs_top(d, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops hv_msi_domain_ops = {
+ .select = msi_lib_irq_domain_select,
+ .alloc = hv_msi_domain_alloc,
+ .free = hv_msi_domain_free,
};
struct irq_domain * __init hv_create_pci_msi_domain(void)
{
struct irq_domain *d = NULL;
- struct fwnode_handle *fn;
- fn = irq_domain_alloc_named_fwnode("HV-PCI-MSI");
- if (fn)
- d = pci_msi_create_irq_domain(fn, &hv_pci_msi_domain_info, x86_vector_domain);
+ struct irq_domain_info info = {
+ .fwnode = irq_domain_alloc_named_fwnode("HV-PCI-MSI"),
+ .ops = &hv_msi_domain_ops,
+ .parent = x86_vector_domain,
+ };
+
+ if (info.fwnode)
+ d = msi_create_parent_irq_domain(&info, &hv_msi_parent_ops);
/* No point in going further if we can't get an irq domain */
BUG_ON(!d);
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 57623ca7f350..9afffedce290 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -10,6 +10,7 @@ config HYPERV
select X86_HV_CALLBACK_VECTOR if X86
select OF_EARLY_FLATTREE if OF
select SYSFB if EFI && !HYPERV_VTL_MODE
+ select IRQ_MSI_LIB if X86
help
Select this option to run Linux as a Hyper-V client operating
system.
--
2.49.0
^ permalink raw reply related
* [PATCH net] net: core: Fix the loop in default_device_exit_net()
From: Haiyang Zhang @ 2025-07-18 20:20 UTC (permalink / raw)
To: linux-hyperv, netdev
Cc: haiyangz, kys, wei.liu, edumazet, kuba, pabeni, horms, davem, sdf,
kuniyu, ahmed.zaki, aleksander.lobakin, linux-kernel, stable, #,
5.4+
From: Haiyang Zhang <haiyangz@microsoft.com>
The loop in default_device_exit_net() won't be able to properly detect the
head then stop, and will hit NULL pointer, when a driver, like hv_netvsc,
automatically moves the slave device together with the master device.
To fix this, add a helper function to return the first migratable netdev
correctly, no matter one or two devices were removed from this net's list
in the last iteration.
Cc: stable@vger.kernel.org # 5.4+
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
net/core/dev.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 621a639aeba1..d83f5f12cf70 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -12629,19 +12629,11 @@ static struct pernet_operations __net_initdata netdev_net_ops = {
.exit = netdev_exit,
};
-static void __net_exit default_device_exit_net(struct net *net)
+static inline struct net_device *first_migratable_netdev(struct net *net)
{
- struct netdev_name_node *name_node, *tmp;
struct net_device *dev, *aux;
- /*
- * Push all migratable network devices back to the
- * initial network namespace
- */
- ASSERT_RTNL();
- for_each_netdev_safe(net, dev, aux) {
- int err;
- char fb_name[IFNAMSIZ];
+ for_each_netdev_safe(net, dev, aux) {
/* Ignore unmoveable devices (i.e. loopback) */
if (dev->netns_immutable)
continue;
@@ -12650,6 +12642,25 @@ static void __net_exit default_device_exit_net(struct net *net)
if (dev->rtnl_link_ops && !dev->rtnl_link_ops->netns_refund)
continue;
+ return dev;
+ }
+
+ return NULL;
+}
+
+static void __net_exit default_device_exit_net(struct net *net)
+{
+ struct netdev_name_node *name_node, *tmp;
+ struct net_device *dev;
+ /*
+ * Push all migratable network devices back to the
+ * initial network namespace
+ */
+ ASSERT_RTNL();
+ while ((dev = first_migratable_netdev(net)) != NULL) {
+ int err;
+ char fb_name[IFNAMSIZ];
+
/* Push remaining network devices to init_net */
snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
if (netdev_name_in_use(&init_net, fb_name))
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox