RE: [PATCH v2 1/2] Drivers: hv: vmbus: Wait for offers during boot
From: Naman Jain Sent: Tuesday, October 29, 2024
1:02 AM
>
> Channel offers are requested during VMBus initialization and resume
> from hibernation. Add support to wait for all channel offers to be
Given the definition of ALLOFFERS_DELIVERED, maybe change to:
" wait for all boot-time channel offers"
> delivered and processed before returning from vmbus_request_offers.
>
> This is in analogy to a PCI bus not returning from probe until it has
> scanned all devices on the bus.
>
> Without this, user mode can race with VMBus initialization and miss
> channel offers. User mode has no way to work around this other than
> sleeping for a while, since there is no way to know when VMBus has
> finished processing offers.
"finished processing boot-time offers."
>
> With this added functionality, remove earlier logic which keeps track
> of count of offered channels post resume from hibernation. Once all
> offers delivered message is received, no further offers are going to
> be received.
"no further boot-time offers"
> Consequently, logic to prevent suspend from happening
> after previous resume had missing offers, is also removed.
>
> Co-developed-by: John Starks
> Signed-off-by: John Starks
> Signed-off-by: Naman Jain
> Reviewed-by: Easwar Hariharan
> ---
> Changes since v1:
> https://lore.kernel.org/all/20241018115811.5530-1-namj...@linux.microsoft.com/
>
> * Added Easwar's Reviewed-By tag
> * Addressed Michael's comments:
> * Added explanation of all offers delivered message in comments
> * Removed infinite wait for offers logic, and changed it wait once.
> * Removed sub channel workqueue flush logic
> * Added comments on why MLX device offer is not expected as part of
> this essential boot offer list. I refrained from adding too many
You've used slightly different phrasing here ("essential boot offer list").
Potential confusion can be avoided if the terminology is super consistent.
I'm good with "boot-time offers" (or something else if you prefer). I'm not
sure what "essential" means here, unless it refers to offers for VFs from
SR-IOV NICs (like Mellanox) being excluded. But it should be fine to
use something short like "boot-time offers" and then note the VF
exception in the code comments as you've done.
> details on it as it felt like it is beyond the scope of this patch
> series and may not be relevant to this. However, please let me know if
> something needs to be added.
> * Addressed Saurabh's comments:
> * Changed timeout value to 1 ms instead of 10*1
> * Changed commit msg as per suggestions
> * Added a comment for warning case of wait_for_completion timeout
> ---
> drivers/hv/channel_mgmt.c | 55 ---
> drivers/hv/connection.c | 4 +--
> drivers/hv/hyperv_vmbus.h | 14 +++---
> drivers/hv/vmbus_drv.c| 16
> 4 files changed, 45 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 3c6011a48dab..a2e9ebe5bf72 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -944,16 +944,6 @@ void vmbus_initiate_unload(bool crash)
> vmbus_wait_for_unload();
> }
>
> -static void check_ready_for_resume_event(void)
> -{
> - /*
> - * If all the old primary channels have been fixed up, then it's safe
> - * to resume.
> - */
> - if (atomic_dec_and_test(&vmbus_connection.nr_chan_fixup_on_resume))
> - complete(&vmbus_connection.ready_for_resume_event);
> -}
> -
> static void vmbus_setup_channel_state(struct vmbus_channel *channel,
> struct vmbus_channel_offer_channel *offer)
> {
> @@ -1109,8 +1099,6 @@ static void vmbus_onoffer(struct
> vmbus_channel_message_header *hdr)
>
> /* Add the channel back to the array of channels. */
> vmbus_channel_map_relid(oldchannel);
> - check_ready_for_resume_event();
> -
> mutex_unlock(&vmbus_connection.channel_mutex);
> return;
> }
> @@ -1296,13 +1284,22 @@
> EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
>
> /*
> * vmbus_onoffers_delivered -
> - * This is invoked when all offers have been delivered.
> + * CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all the essential
Again, I would drop "essential" here, as its meaning in this context isn't
defined anywhere.
> + * boot-time offers are delivered. Other channels can be hot added
> + * or removed later, even immediately after the all-offers-delivered
> + * message. A boot-time offer will be any of the virtual hardware the
> + * VM is configured with at boot.
This is good to have a definition of "boot-time offer". But I think some
additional specification is appropriate. Let me suggest the following:
The CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all
boot-time offers are delivered. A boot-time offer is for the primary channel
for any virtual hardware configured i
RE: [PATCH v2 2/2] Drivers: hv: vmbus: Log on missing offers
From: Naman Jain Sent: Tuesday, October 29, 2024
1:02 AM
>
> When resuming from hibernation, log any channels that were present
> before hibernation but now are gone.
> In general, the essential virtual devices configured for a VM, remain
> same, before and after the hibernation and its not very common that
> some offers are missing.
The wording here is a bit jumbled. And let's use consistent terminology.
I'd suggest:
In general, the boot-time devices configured for a resuming VM should be
the same as the devices in the VM at the time of hibernation. It's uncommon
for the configuration to have been changed such that offers are missing.
Changing the configuration violates the rules for hibernation anyway.
> The cleanup of missing channels is not
> straight-forward and dependent on individual device driver
> functionality and implementation, so it can be added in future as
> separate changes.
>
> Signed-off-by: John Starks
> Co-developed-by: Naman Jain
> Signed-off-by: Naman Jain
> Reviewed-by: Easwar Hariharan
> ---
> Changes since v1:
> https://lore.kernel.org/all/20241018115811.5530-1-namj...@linux.microsoft.com/
> * Added Easwar's Reviewed-By tag
> * Addressed Saurabh's comments:
> * Added a note for missing channel cleanup in comments and commit msg
> ---
> drivers/hv/vmbus_drv.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index bd3fc41dc06b..08214f28694a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2462,6 +2462,7 @@ static int vmbus_bus_suspend(struct device *dev)
>
> static int vmbus_bus_resume(struct device *dev)
> {
> + struct vmbus_channel *channel;
> struct vmbus_channel_msginfo *msginfo;
> size_t msgsize;
> int ret;
> @@ -2494,6 +2495,22 @@ static int vmbus_bus_resume(struct device *dev)
>
> vmbus_request_offers();
>
> + mutex_lock(&vmbus_connection.channel_mutex);
> + list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
> + if (channel->offermsg.child_relid != INVALID_RELID)
> + continue;
> +
> + /* hvsock channels are not expected to be present. */
> + if (is_hvsock_channel(channel))
> + continue;
> +
> + pr_err("channel %pUl/%pUl not present after resume.\n",
> + &channel->offermsg.offer.if_type,
> + &channel->offermsg.offer.if_instance);
> + /* ToDo: Cleanup these channels here */
> + }
> + mutex_unlock(&vmbus_connection.channel_mutex);
> +
Dexuan and John have explained how in Azure VMs, there should not be
any VFs assigned to the VM at the time of hibernation. So the above
check for missing offers does not trigger an error message due to
VF offers coming after the all-offers-received message.
But what about the case of a VM running on a local Hyper-V? I'm not
completely clear, but in that case I don't think any VFs are removed
before the hibernation, especially for VM-initiated hibernation. It's
a reasonable scenario to later resume that same VM, with the same
VF assigned to the VM. Because of the way current code counts
the offers, vmbus_bus_resume() waits for the VF to be offered again,
and all the channels get correct post-resume relids. But the changes
in this patch set break that scenario. Since vmbus_bus_resume() now
proceeds before the VF offer arrives, hv_pci_resume() calling
vmbus_open() could use the pre-hibernation relid for the VF and break
things. Certainly the "not present after resume" error message would
be spurious.
Maybe the focus here is Azure, and it's tolerable for the local Hyper-V
case with a VF to not work pending later fixes. But I thought I'd call
out the potential issue (assuming my thinking is correct).
Michael
> /* Reset the event for the next suspend. */
> reinit_completion(&vmbus_connection.ready_for_suspend_event);
>
> --
> 2.34.1
Re: [PATCH 2/5] hyperv: Remove unnecessary #includes
On 10/10/2024 11:21 AM, Michael Kelley wrote: > From: Nuno Das Neves Sent: Thursday, > October 3, 2024 12:51 PM >> >> asm/hyperv-tlfs.h is already included implicitly wherever mshyperv.h >> or linux/hyperv.h is included. Remove those redundancies. > > I've been under the impression that it is preferable to directly include > .h files for all definitions that a source code file uses, even if the > needed .h file is indirectly included by some other .h file. I looked through I've also heard this idea, and generally try to stick to it. I myself haven't yet encountered a situation where doing this or not has greatly impacted the readability of the code. > the coding style documentation, and didn't find anything addressing > this topic, so maybe I'm wrong. But I know I've seen patches to other > parts of the kernel that were changes to follow the direct inclusion > approach. Direct inclusion is less fragile if the #include file nesting > structure changes (and perhaps removes the indirect inclusion). > > The mshyperv.h and linux/hyperv.h dependency on hyperv-tlfs.h is > highly unlikely to change, so the chance of breakage is minimal. But You probably already understood this after seeing the later patches, but it's worth pointing out that the goal of this series is to change that dependency (situationally), which is exactly why I created this precursor patch. > I wonder if your approach is going the wrong direction vs. preferred > kernel practices. > I could replace with in most of these cases (in v2), to preserve the explicit include of the definitions before mshyperv.h or linux/hyperv.h. I will give it a try in v2. Thanks, Nuno > Michael > >> >> Remove includes of linux/hyperv.h and mshyperv.h where they are not >> needed. >> >> Signed-off-by: Nuno Das Neves >> --- >> arch/arm64/hyperv/hv_core.c| 2 -- >> arch/x86/hyperv/hv_apic.c | 1 - >> arch/x86/hyperv/hv_init.c | 2 -- >> arch/x86/hyperv/hv_proc.c | 1 - >> arch/x86/hyperv/ivm.c | 1 - >> arch/x86/hyperv/mmu.c | 1 - >> arch/x86/hyperv/nested.c | 1 - >> arch/x86/include/asm/kvm_host.h| 1 - >> arch/x86/include/asm/mshyperv.h| 1 - >> arch/x86/kernel/cpu/mshyperv.c | 1 - >> arch/x86/kvm/vmx/vmx_onhyperv.h| 1 - >> arch/x86/mm/pat/set_memory.c | 2 -- >> drivers/clocksource/hyperv_timer.c | 1 - >> drivers/hv/hv_balloon.c| 2 -- >> drivers/hv/hv_common.c | 1 - >> drivers/hv/hv_kvp.c| 1 - >> drivers/hv/hv_snapshot.c | 1 - >> drivers/hv/hyperv_vmbus.h | 1 - >> net/vmw_vsock/hyperv_transport.c | 1 - >> 19 files changed, 23 deletions(-) >> >> diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c >> index f1ebc025e1df..9d1969b875e9 100644 >> --- a/arch/arm64/hyperv/hv_core.c >> +++ b/arch/arm64/hyperv/hv_core.c >> @@ -11,11 +11,9 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> -#include >> #include >> >> /* >> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c >> index 0569f579338b..f022d5f64fb6 100644 >> --- a/arch/x86/hyperv/hv_apic.c >> +++ b/arch/x86/hyperv/hv_apic.c >> @@ -23,7 +23,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c >> index 17a71e92a343..fc3c3d76c181 100644 >> --- a/arch/x86/hyperv/hv_init.c >> +++ b/arch/x86/hyperv/hv_init.c >> @@ -19,7 +19,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> @@ -27,7 +26,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> diff --git a/arch/x86/hyperv/hv_proc.c b/arch/x86/hyperv/hv_proc.c >> index 3fa1f2ee7b0d..b74c06c04ff1 100644 >> --- a/arch/x86/hyperv/hv_proc.c >> +++ b/arch/x86/hyperv/hv_proc.c >> @@ -3,7 +3,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c >> index 60fc3ed72830..b56d70612734 100644 >> --- a/arch/x86/hyperv/ivm.c >> +++ b/arch/x86/hyperv/ivm.c >> @@ -7,7 +7,6 @@ >> */ >> >> #include >> -#include >> #include >> #include >> #include >> diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c >> index 1cc113200ff5..cc8c3bd0e7c2 100644 >> --- a/arch/x86/hyperv/mmu.c >> +++ b/arch/x86/hyperv/mmu.c >> @@ -1,6 +1,5 @@ >> #define pr_fmt(fmt) "Hyper-V: " fmt >> >> -#include >> #include >> #include >> #include >> diff --git a/arch/x86/hyperv/nested.c b/arch/x86/hyperv/nested.c >> index 9dc259fa322e..ee06d0315c24 100644 >> --- a/arch/x86/hyperv/nested.c >> +++ b/arch/x86/hyperv/nested.c >> @@ -11,7 +11,6 @@ >> >> >> #include >> -#include >> #include >> #include >> >> diff --git a/arch/x86/include/asm/kvm_host.h >> b/arch/x86/include/asm/kvm_host.h >> index 4a68c
RE: [PATCH v3 1/2] jiffies: Define secs_to_jiffies()
> -Original Message- > From: Easwar Hariharan > Sent: Wednesday, October 30, 2024 1:48 PM > To: KY Srinivasan ; Haiyang Zhang > ; Wei Liu ; Dexuan Cui > ; linux-hyperv@vger.kernel.org; Anna-Maria Behnsen > ; Thomas Gleixner ; Geert > Uytterhoeven ; Marcel Holtmann > ; Johan Hedberg ; Luiz > Augusto von Dentz ; linux- > blueto...@vger.kernel.org; linux-ker...@vger.kernel.org; Praveen Kumar > ; Naman Jain > > Cc: Michael Kelley ; Easwar Hariharan > ; Von Dentz, Luiz > > Subject: [PATCH v3 1/2] jiffies: Define secs_to_jiffies() > > secs_to_jiffies() is defined in hci_event.c and cannot be reused by > other call sites. Hoist it into the core code to allow conversion of the > ~1150 usages of msecs_to_jiffies() that either: > - use a multiplier value of 1000 or equivalently MSEC_PER_SEC, or > - have timeouts that are denominated in seconds (i.e. end in 000) > > This will also allow conversion of yet more sites that use (sec * HZ) > directly, and improve their readability. > > TO: K. Y. Srinivasan > TO: Haiyang Zhang > TO: Wei Liu > TO: Dexuan Cui > TO: linux-hyperv@vger.kernel.org > TO: Anna-Maria Behnsen > TO: Thomas Gleixner > TO: Geert Uytterhoeven > TO: Marcel Holtmann > TO: Johan Hedberg > TO: Luiz Augusto von Dentz > TO: linux-blueto...@vger.kernel.org > TO: linux-ker...@vger.kernel.org > Suggested-by: Michael Kelley > Reviewed-by: Luiz Augusto von Dentz > Signed-off-by: Easwar Hariharan > --- > include/linux/jiffies.h | 12 > net/bluetooth/hci_event.c | 2 -- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h > index > 1220f0fbe5bf9fb6c559b4efd603db3e97db9b65..e17c220ed56e587fd55fb9cf4a133a5 > 3588af940 100644 > --- a/include/linux/jiffies.h > +++ b/include/linux/jiffies.h > @@ -526,6 +526,18 @@ static __always_inline unsigned long > msecs_to_jiffies(const unsigned int m) > } > } > > +/** > + * secs_to_jiffies: - convert seconds to jiffies > + * @_secs: time in seconds > + * > + * Conversion is done by simple multiplication with HZ > + * secs_to_jiffies() is defined as a macro rather than a static inline > + * function due to its potential application in struct initializers. > + * > + * Return: jiffies value > + */ > +#define secs_to_jiffies(_secs) ((_secs) * HZ) > + > extern unsigned long __usecs_to_jiffies(const unsigned int u); > #if !(USEC_PER_SEC % HZ) > static inline unsigned long _usecs_to_jiffies(const unsigned int u) > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index > 0bbad90ddd6f87e87c03859bae48a7901d39b634..7b35c58bbbeb79f2b50a02212771fb2 > 83ba5643d 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -42,8 +42,6 @@ > #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \ >"\x00\x00\x00\x00\x00\x00\x00\x00" > > -#define secs_to_jiffies(_secs) msecs_to_jiffies((_secs) * 1000) > - > /* Handle HCI Event packets */ > > static void *hci_ev_skb_pull(struct hci_dev *hdev, struct sk_buff *skb, > > -- > 2.34.1 All looks good. But can you consider naming the macro as s2jiffy()? Just to be shorter, so after adopting this macro we don't have to split some lines for over 80 characters:) Thanks, - Haiyang
Re: [PATCH v3 1/2] jiffies: Define secs_to_jiffies()
On 10/31/2024 8:54 AM, Haiyang Zhang wrote: > > >> -Original Message- >> From: Easwar Hariharan >> Sent: Wednesday, October 30, 2024 1:48 PM >> To: KY Srinivasan ; Haiyang Zhang >> ; Wei Liu ; Dexuan Cui >> ; linux-hyperv@vger.kernel.org; Anna-Maria Behnsen >> ; Thomas Gleixner ; Geert >> Uytterhoeven ; Marcel Holtmann >> ; Johan Hedberg ; Luiz >> Augusto von Dentz ; linux- >> blueto...@vger.kernel.org; linux-ker...@vger.kernel.org; Praveen Kumar >> ; Naman Jain >> >> Cc: Michael Kelley ; Easwar Hariharan >> ; Von Dentz, Luiz >> >> Subject: [PATCH v3 1/2] jiffies: Define secs_to_jiffies() >> >> secs_to_jiffies() is defined in hci_event.c and cannot be reused by >> other call sites. Hoist it into the core code to allow conversion of the >> ~1150 usages of msecs_to_jiffies() that either: >> - use a multiplier value of 1000 or equivalently MSEC_PER_SEC, or >> - have timeouts that are denominated in seconds (i.e. end in 000) >> >> This will also allow conversion of yet more sites that use (sec * HZ) >> directly, and improve their readability. >> >> TO: K. Y. Srinivasan >> TO: Haiyang Zhang >> TO: Wei Liu >> TO: Dexuan Cui >> TO: linux-hyperv@vger.kernel.org >> TO: Anna-Maria Behnsen >> TO: Thomas Gleixner >> TO: Geert Uytterhoeven >> TO: Marcel Holtmann >> TO: Johan Hedberg >> TO: Luiz Augusto von Dentz >> TO: linux-blueto...@vger.kernel.org >> TO: linux-ker...@vger.kernel.org >> Suggested-by: Michael Kelley >> Reviewed-by: Luiz Augusto von Dentz >> Signed-off-by: Easwar Hariharan >> --- >> include/linux/jiffies.h | 12 >> net/bluetooth/hci_event.c | 2 -- >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h >> index >> 1220f0fbe5bf9fb6c559b4efd603db3e97db9b65..e17c220ed56e587fd55fb9cf4a133a5 >> 3588af940 100644 >> --- a/include/linux/jiffies.h >> +++ b/include/linux/jiffies.h >> @@ -526,6 +526,18 @@ static __always_inline unsigned long >> msecs_to_jiffies(const unsigned int m) >> } >> } >> >> +/** >> + * secs_to_jiffies: - convert seconds to jiffies >> + * @_secs: time in seconds >> + * >> + * Conversion is done by simple multiplication with HZ >> + * secs_to_jiffies() is defined as a macro rather than a static inline >> + * function due to its potential application in struct initializers. >> + * >> + * Return: jiffies value >> + */ >> +#define secs_to_jiffies(_secs) ((_secs) * HZ) >> + >> extern unsigned long __usecs_to_jiffies(const unsigned int u); >> #if !(USEC_PER_SEC % HZ) >> static inline unsigned long _usecs_to_jiffies(const unsigned int u) >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index >> 0bbad90ddd6f87e87c03859bae48a7901d39b634..7b35c58bbbeb79f2b50a02212771fb2 >> 83ba5643d 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -42,8 +42,6 @@ >> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \ >> "\x00\x00\x00\x00\x00\x00\x00\x00" >> >> -#define secs_to_jiffies(_secs) msecs_to_jiffies((_secs) * 1000) >> - >> /* Handle HCI Event packets */ >> >> static void *hci_ev_skb_pull(struct hci_dev *hdev, struct sk_buff *skb, >> >> -- >> 2.34.1 > > All looks good. > But can you consider naming the macro as s2jiffy()? Just > to be shorter, so after adopting this macro we don't have > to split some lines for over 80 characters:) > > Thanks, > - Haiyang > Thanks for the review! The patch introducing the macro has already been accepted into timers/core in tip[1], so unfortunately I can't make that change anymore. For readability considerations, I also find it better to match the remaining APIs in the jiffies family, i.e. msecs_to_jiffies(), nsecs_to_jiffies(), usecs_to_jiffies(). [1] https://git.kernel.org/tip/b35108a51cf7bab58d7eace1267d7965978bcdb8 Thanks, Easwar
Re: [PATCH 0/5] Add new headers for Hyper-V Dom0
On 10/3/2024 12:50 PM, Nuno Das Neves wrote: > To support Hyper-V Dom0 (aka Linux as root partition), many new > definitions are required. > > The plan going forward is to directly import headers from > Hyper-V. This is a more maintainable way to import definitions > rather than via the TLFS doc. This patch series introduces > new headers (hvhdk.h, hvgdk.h, etc, see patch #3) directly > derived from Hyper-V code. > > This patch series replaces hyperv-tlfs.h with hvhdk.h, but only > in Microsoft-maintained Hyper-V code where they are needed. This > leaves the existing hyperv-tlfs.h in use elsewhere - notably for > Hyper-V enlightenments on KVM guests. > > An intermediary header "hv_defs.h" is introduced to conditionally > include either hyperv-tlfs.h or hvhdk.h. This is required because > several headers which today include hyperv-tlfs.h, are shared > between Hyper-V and KVM code (e.g. mshyperv.h). > > Summary: > Patch 1-2: Cleanup patches > Patch 3: Add the new headers (hvhdk.h, etc..) in include/hyperv/ > Patch 4: Add hv_defs.h and use it in mshyperv.h, svm.h, > hyperv_timer.h > Patch 5: Switch to the new headers, only in Hyper-V code > > Nuno Das Neves (5): > hyperv: Move hv_connection_id to hyperv-tlfs.h > hyperv: Remove unnecessary #includes > hyperv: Add new Hyper-V headers > hyperv: Add hv_defs.h to conditionally include hyperv-tlfs.h or > hvhdk.h > hyperv: Use hvhdk.h instead of hyperv-tlfs.h in Hyper-V code > What is the model for Hyper-V code that has both guest and host roles where the corresponding hypercalls are available for both? As I understand it, those are supposed to be in hvgdk*.h. For a specific example, IOMMU hypercalls can operate on stage 2 or stage 1 translations depending on the role of the (hyper) caller and the input values provided. Should a driver using these hypercalls import both hvhdk* and hvgdk*? What about hyperv-tlfs? Patches 4 and 5 seem to draw a bright line between host and guest roles while the reality is more gray. Please do correct me if I'm wrong here, perhaps the picture would be clearer if Stas' suggestion of a new header file is implemented. Thanks, Easwar
RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet
> From: Michael Kelley
> Sent: Wednesday, October 30, 2024 5:12 PM
> [...]
> What do you think about this (compile tested only), which splits the
> "init" function into two parts for devices that have char devs? I'm
> trying to avoid adding yet another synchronization point by just
> doing the init operations in the right order -- i.e., don't create the
> user space /dev entry until the VMBus channel is ready.
>
> Michael
Thanks, I think this works! This is a better fix.
> + if (srv->util_init_transport) {
> + ret = srv->util_init_transport();
> + if (ret) {
> + ret = -ENODEV;
IMO we don't need the line above, since the 'ret' from
srv->util_init_transport() is already a standard error code.
BTW, I noticed that the line "ret = -ENODEV;"
if (srv->util_init) {
ret = srv->util_init(srv);
if (ret) {
ret = -ENODEV;
goto error1;
}
}
I think we don't really need that line, either.
The existing 4 .util_init callbacks also already return a
standard error code. We can make a separate patch to clean
that up.
Thanks,
Dexuan
RE: [PATCH v3 1/2] jiffies: Define secs_to_jiffies()
> -Original Message- > From: Easwar Hariharan > Sent: Thursday, October 31, 2024 1:06 PM > To: Haiyang Zhang ; KY Srinivasan > ; Wei Liu ; Dexuan Cui > ; linux-hyperv@vger.kernel.org; Anna-Maria Behnsen > ; Thomas Gleixner ; Geert > Uytterhoeven ; Marcel Holtmann > ; Johan Hedberg ; Luiz > Augusto von Dentz ; linux- > blueto...@vger.kernel.org; linux-ker...@vger.kernel.org; Praveen Kumar > ; Naman Jain > > Cc: eahar...@linux.microsoft.com; Michael Kelley ; > Von Dentz, Luiz > Subject: Re: [PATCH v3 1/2] jiffies: Define secs_to_jiffies() > > On 10/31/2024 8:54 AM, Haiyang Zhang wrote: > > > > > >> -Original Message- > >> From: Easwar Hariharan > >> Sent: Wednesday, October 30, 2024 1:48 PM > >> To: KY Srinivasan ; Haiyang Zhang > >> ; Wei Liu ; Dexuan Cui > >> ; linux-hyperv@vger.kernel.org; Anna-Maria > Behnsen > >> ; Thomas Gleixner ; > Geert > >> Uytterhoeven ; Marcel Holtmann > >> ; Johan Hedberg ; Luiz > >> Augusto von Dentz ; linux- > >> blueto...@vger.kernel.org; linux-ker...@vger.kernel.org; Praveen Kumar > >> ; Naman Jain > >> > >> Cc: Michael Kelley ; Easwar Hariharan > >> ; Von Dentz, Luiz > >> > >> Subject: [PATCH v3 1/2] jiffies: Define secs_to_jiffies() > >> > >> secs_to_jiffies() is defined in hci_event.c and cannot be reused by > >> other call sites. Hoist it into the core code to allow conversion of > the > >> ~1150 usages of msecs_to_jiffies() that either: > >> - use a multiplier value of 1000 or equivalently MSEC_PER_SEC, or > >> - have timeouts that are denominated in seconds (i.e. end in 000) > >> > >> This will also allow conversion of yet more sites that use (sec * HZ) > >> directly, and improve their readability. > >> > >> TO: K. Y. Srinivasan > >> TO: Haiyang Zhang > >> TO: Wei Liu > >> TO: Dexuan Cui > >> TO: linux-hyperv@vger.kernel.org > >> TO: Anna-Maria Behnsen > >> TO: Thomas Gleixner > >> TO: Geert Uytterhoeven > >> TO: Marcel Holtmann > >> TO: Johan Hedberg > >> TO: Luiz Augusto von Dentz > >> TO: linux-blueto...@vger.kernel.org > >> TO: linux-ker...@vger.kernel.org > >> Suggested-by: Michael Kelley > >> Reviewed-by: Luiz Augusto von Dentz > >> Signed-off-by: Easwar Hariharan > >> --- > >> include/linux/jiffies.h | 12 > >> net/bluetooth/hci_event.c | 2 -- > >> 2 files changed, 12 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h > >> index > >> > 1220f0fbe5bf9fb6c559b4efd603db3e97db9b65..e17c220ed56e587fd55fb9cf4a133a5 > >> 3588af940 100644 > >> --- a/include/linux/jiffies.h > >> +++ b/include/linux/jiffies.h > >> @@ -526,6 +526,18 @@ static __always_inline unsigned long > >> msecs_to_jiffies(const unsigned int m) > >>} > >> } > >> > >> +/** > >> + * secs_to_jiffies: - convert seconds to jiffies > >> + * @_secs: time in seconds > >> + * > >> + * Conversion is done by simple multiplication with HZ > >> + * secs_to_jiffies() is defined as a macro rather than a static > inline > >> + * function due to its potential application in struct initializers. > >> + * > >> + * Return: jiffies value > >> + */ > >> +#define secs_to_jiffies(_secs) ((_secs) * HZ) > >> + > >> extern unsigned long __usecs_to_jiffies(const unsigned int u); > >> #if !(USEC_PER_SEC % HZ) > >> static inline unsigned long _usecs_to_jiffies(const unsigned int u) > >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > >> index > >> > 0bbad90ddd6f87e87c03859bae48a7901d39b634..7b35c58bbbeb79f2b50a02212771fb2 > >> 83ba5643d 100644 > >> --- a/net/bluetooth/hci_event.c > >> +++ b/net/bluetooth/hci_event.c > >> @@ -42,8 +42,6 @@ > >> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \ > >> "\x00\x00\x00\x00\x00\x00\x00\x00" > >> > >> -#define secs_to_jiffies(_secs) msecs_to_jiffies((_secs) * 1000) > >> - > >> /* Handle HCI Event packets */ > >> > >> static void *hci_ev_skb_pull(struct hci_dev *hdev, struct sk_buff > *skb, > >> > >> -- > >> 2.34.1 > > > > All looks good. > > But can you consider naming the macro as s2jiffy()? Just > > to be shorter, so after adopting this macro we don't have > > to split some lines for over 80 characters:) > > > > Thanks, > > - Haiyang > > > > Thanks for the review! The patch introducing the macro has already been > accepted into timers/core in tip[1], so unfortunately I can't make that > change anymore. For readability considerations, I also find it better to > match the remaining APIs in the jiffies family, i.e. msecs_to_jiffies(), > nsecs_to_jiffies(), usecs_to_jiffies(). > > [1] > https://git.ker/ > nel.org%2Ftip%2Fb35108a51cf7bab58d7eace1267d7965978bcdb8&data=05%7C02%7Ch > aiyangz%40microsoft.com%7C7d5db079ed124f62ac2a08dcf9ce4b20%7C72f988bf86f1 > 41af91ab2d7cd011db47%7C1%7C0%7C638659911651280804%7CUnknown%7CTWFpbGZsb3d > 8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFp > bCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=hz5UtwU9CLw068z4tpr9kPMANntwX58De > A5dXi9pqSg%3D&reserved=0 > Then, that's fine. Thanks - Haiyang
Re: [PATCH 0/5] Add new headers for Hyper-V Dom0
On 10/31/2024 12:05 PM, Easwar Hariharan wrote: > On 10/3/2024 12:50 PM, Nuno Das Neves wrote: >> To support Hyper-V Dom0 (aka Linux as root partition), many new >> definitions are required. >> >> The plan going forward is to directly import headers from >> Hyper-V. This is a more maintainable way to import definitions >> rather than via the TLFS doc. This patch series introduces >> new headers (hvhdk.h, hvgdk.h, etc, see patch #3) directly >> derived from Hyper-V code. >> >> This patch series replaces hyperv-tlfs.h with hvhdk.h, but only >> in Microsoft-maintained Hyper-V code where they are needed. This >> leaves the existing hyperv-tlfs.h in use elsewhere - notably for >> Hyper-V enlightenments on KVM guests. >> >> An intermediary header "hv_defs.h" is introduced to conditionally >> include either hyperv-tlfs.h or hvhdk.h. This is required because >> several headers which today include hyperv-tlfs.h, are shared >> between Hyper-V and KVM code (e.g. mshyperv.h). >> >> Summary: >> Patch 1-2: Cleanup patches >> Patch 3: Add the new headers (hvhdk.h, etc..) in include/hyperv/ >> Patch 4: Add hv_defs.h and use it in mshyperv.h, svm.h, >> hyperv_timer.h >> Patch 5: Switch to the new headers, only in Hyper-V code >> >> Nuno Das Neves (5): >> hyperv: Move hv_connection_id to hyperv-tlfs.h >> hyperv: Remove unnecessary #includes >> hyperv: Add new Hyper-V headers >> hyperv: Add hv_defs.h to conditionally include hyperv-tlfs.h or >> hvhdk.h >> hyperv: Use hvhdk.h instead of hyperv-tlfs.h in Hyper-V code >> Hi Easwar, thanks for the questions. I will attempt to clarify. > > What is the model for Hyper-V code that has both guest and host roles > where the corresponding hypercalls are available for both? As I > understand it, those are supposed to be in hvgdk*.h. > It's true that the naming of the files implies hvgdk*.h is for guests, and hvhdk*.h (which includes hvgdk*.h), is for hosts/dom0. But I would only take that as a rough guide. The real reason for keeping these names is to make it a easier to copy and maintain the definitions from the Windows code into Linux, by keeping the provenance of exactly where they came from. > For a specific example, IOMMU hypercalls can operate on stage 2 or stage > 1 translations depending on the role of the (hyper) caller and the input > values provided. Should a driver using these hypercalls import both > hvhdk* and hvgdk*? What about hyperv-tlfs? > I'd recommend importing hvhdk.h since it contains everything you need (including hvgdk*.h). The goal of this patchset is to move away from hyperv-tlfs.h, because by definition it should only contain definitions from the TLFS document. > Patches 4 and 5 seem to draw a bright line between host and guest roles > while the reality is more gray. Please do correct me if I'm wrong here, > perhaps the picture would be clearer if Stas' suggestion of a new header > file is implemented. > Patches 4 and 5 introduce the new headers in a way that avoids any potential impact on KVM and other non-Microsoft-maintained code. The 'line' is not between guest and host, but between Microsoft-maintained and non-Microsoft-maintained code. Thanks, Nuno > Thanks, > Easwar
RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet
From: Dexuan Cui Sent: Thursday, October 31, 2024 5:17 PM
>
> > From: Michael Kelley
> > Sent: Wednesday, October 30, 2024 5:12 PM
> > [...]
> > What do you think about this (compile tested only), which splits the
> > "init" function into two parts for devices that have char devs? I'm
> > trying to avoid adding yet another synchronization point by just
> > doing the init operations in the right order -- i.e., don't create the
> > user space /dev entry until the VMBus channel is ready.
> >
> > Michael
>
> Thanks, I think this works! This is a better fix.
>
> > + if (srv->util_init_transport) {
> > + ret = srv->util_init_transport();
> > + if (ret) {
> > + ret = -ENODEV;
> IMO we don't need the line above, since the 'ret' from
> srv->util_init_transport() is already a standard error code.
I was just now looking at call_driver_probe(), and it behaves
slightly differently for ENODEV and ENXIO vs. other error
codes. ENODEV and ENXIO don't output a message to the
console unless debugging is enabled, while other error codes
always output a message to the console. Forcing the error to
ENODEV has been there since the util_probe() code came out
of staging in year 2011. But I don't really have a preference
either way.
>
> BTW, I noticed that the line "ret = -ENODEV;"
> if (srv->util_init) {
> ret = srv->util_init(srv);
> if (ret) {
> ret = -ENODEV;
> goto error1;
> }
> }
> I think we don't really need that line, either.
> The existing 4 .util_init callbacks also already return a
> standard error code. We can make a separate patch to clean
> that up.
Same here.
Michael
