RE: [PATCH v2 2/5] x86: add enum for hypervisors to replace x86_hyper
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Friday, November 10, 2017 10:26 AM
> To: Juergen Gross <jgr...@suse.com>
> Cc: linux-ker...@vger.kernel.org; t...@linutronix.de; mi...@redhat.com;
> h...@zytor.com; x...@kernel.org; KY Srinivasan <k...@microsoft.com>;
> Haiyang Zhang <haiya...@microsoft.com>; Stephen Hemminger
> <sthem...@microsoft.com>; akata...@vmware.com;
> pbonz...@redhat.com; rkrc...@redhat.com; boris.ostrov...@oracle.com;
> de...@linuxdriverproject.org; virtualization@lists.linux-foundation.org;
> k...@vger.kernel.org; xen-de...@lists.xenproject.org; linux-graphics-
> maintai...@vmware.com; pv-driv...@vmware.com;
> xdeguill...@vmware.com; moltm...@vmware.com; a...@arndb.de;
> gre...@linuxfoundation.org; linux-in...@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] x86: add enum for hypervisors to replace
> x86_hyper
>
> On Thu, Nov 09, 2017 at 02:27:36PM +0100, Juergen Gross wrote:
> > The x86_hyper pointer is only used for checking whether a virtual
> > device is supporting the hypervisor the system is running on.
> >
> > Use an enum for that purpose instead and drop the x86_hyper pointer.
> >
> > Cc: k...@microsoft.com
> > Cc: haiya...@microsoft.com
> > Cc: sthem...@microsoft.com
> > Cc: akata...@vmware.com
> > Cc: pbonz...@redhat.com
> > Cc: rkrc...@redhat.com
> > Cc: boris.ostrov...@oracle.com
> > Cc: de...@linuxdriverproject.org
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: k...@vger.kernel.org
> > Cc: xen-de...@lists.xenproject.org
> > Cc: linux-graphics-maintai...@vmware.com
> > Cc: pv-driv...@vmware.com
> > Cc: dmitry.torok...@gmail.com
> > Cc: xdeguill...@vmware.com
> > Cc: moltm...@vmware.com
> > Cc: a...@arndb.de
> > Cc: gre...@linuxfoundation.org
> > Cc: linux-in...@vger.kernel.org
> >
> > Signed-off-by: Juergen Gross <jgr...@suse.com>
> > ---
> > arch/x86/hyperv/hv_init.c | 2 +-
> > arch/x86/include/asm/hypervisor.h | 23 ++-
> > arch/x86/kernel/cpu/hypervisor.c | 12 +---
> > arch/x86/kernel/cpu/mshyperv.c| 4 ++--
> > arch/x86/kernel/cpu/vmware.c | 4 ++--
> > arch/x86/kernel/kvm.c | 4 ++--
> > arch/x86/xen/enlighten_hvm.c | 4 ++--
> > arch/x86/xen/enlighten_pv.c | 4 ++--
> > drivers/hv/vmbus_drv.c| 2 +-
> > drivers/input/mouse/vmmouse.c | 10 --
>
> Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
Acked-by: K. Y. Srinivasan <k...@microsoft.com>
>
> > drivers/misc/vmw_balloon.c| 2 +-
> > 11 files changed, 40 insertions(+), 31 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index a5db63f728a2..a0b86cf486e0 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -113,7 +113,7 @@ void hyperv_init(void)
> > u64 guest_id;
> > union hv_x64_msr_hypercall_contents hypercall_msr;
> >
> > - if (x86_hyper != _hyper_ms_hyperv)
> > + if (x86_hyper_type != X86_HYPER_MS_HYPERV)
> > return;
> >
> > /* Allocate percpu VP index */
> > diff --git a/arch/x86/include/asm/hypervisor.h
> b/arch/x86/include/asm/hypervisor.h
> > index 0eca7239a7aa..1b0a5abcd8ae 100644
> > --- a/arch/x86/include/asm/hypervisor.h
> > +++ b/arch/x86/include/asm/hypervisor.h
> > @@ -29,6 +29,16 @@
> > /*
> > * x86 hypervisor information
> > */
> > +
> > +enum x86_hypervisor_type {
> > + X86_HYPER_NATIVE = 0,
> > + X86_HYPER_VMWARE,
> > + X86_HYPER_MS_HYPERV,
> > + X86_HYPER_XEN_PV,
> > + X86_HYPER_XEN_HVM,
> > + X86_HYPER_KVM,
> > +};
> > +
> > struct hypervisor_x86 {
> > /* Hypervisor name */
> > const char *name;
> > @@ -36,6 +46,9 @@ struct hypervisor_x86 {
> > /* Detection routine */
> > uint32_t(*detect)(void);
> >
> > + /* Hypervisor type */
> > + enum x86_hypervisor_type type;
> > +
> > /* init time callbacks */
> > struct x86_hyper_init init;
> >
> > @@ -43,15 +56,7 @@ struct hypervisor_x86 {
> > struct x86_hyper_runtime runtime;
> > };
> >
> > -extern const struct hypervisor_x86 *x86_hyper;
> > -
> > -/* Recognized hypervisors */
> > -extern const struct hypervisor_x86 x86_hyper_vmware;
> > -extern const struct hypervisor_x86 x86_hyper_ms_hyperv;
> > -extern const struct hypervisor_x86 x86_hyper
RE: [PATCH v2 0/3] x86/vdso: Add Hyper-V TSC page clocksource support
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Tuesday, February 14, 2017 4:44 AM > To: x...@kernel.org; Andy Lutomirski <l...@amacapital.net> > Cc: Thomas Gleixner <t...@linutronix.de>; Ingo Molnar <mi...@redhat.com>; > H. Peter Anvin <h...@zytor.com>; KY Srinivasan <k...@microsoft.com>; > Haiyang Zhang <haiya...@microsoft.com>; Stephen Hemminger > <sthem...@microsoft.com>; Dexuan Cui <de...@microsoft.com>; linux- > ker...@vger.kernel.org; de...@linuxdriverproject.org; > virtualization@lists.linux-foundation.org > Subject: [PATCH v2 0/3] x86/vdso: Add Hyper-V TSC page clocksource support > > Hi, > > while we're still waiting for a definitive ACK from Microsoft that the > algorithm > is good for SMP case (as we can't prevent the code in vdso from migrating > between CPUs) I'd like to send v2 with some modifications to keep the > discussion going. I checked with the folks on the Hyper-V side and they have confirmed that we need to add memory barriers in the guest code to ensure the various reads from the TSC page are correctly ordered - especially, the initial read of the sequence counter must have acquire semantics. We should ensure that other reads from the TSC page are completed before the second read of the sequence counter. I am working with the Windows team to correctly reflect this algorithm in the Hyper-V specification. Regards, K. Y > > Changes since v1: > - Document the TSC page reading protocol [Thomas Gleixner]. > > - Separate the TSC page reading code from read_hv_clock_tsc() and put it to > asm/mshyperv.h to use from both hv_init.c and vdso. > > - Add explicit barriers [Thomas Gleixner] > > Original description: > > Hyper-V TSC page clocksource is suitable for vDSO, however, the protocol > defined by the hypervisor is different from VCLOCK_PVCLOCK. Implemented > the required support. Simple sysbench test shows the following results: > > Before: > # time sysbench --test=memory --max-requests=50 run ... > real1m22.618s > user0m50.193s > sys 0m32.268s > > After: > # time sysbench --test=memory --max-requests=50 run ... > real 0m47.241s > user 0m47.117s > sys 0m0.008s > > Patches 1 and 2 are made on top of K. Y.'s code refactoring which moved tsc > page clocksource to arch/x86/hyperv/hv_init.c, this is currently present in > Greg's char-misc-next tree. > > Vitaly Kuznetsov (3): > x86/hyperv: implement hv_get_tsc_page() > x86/hyperv: move TSC reading method to asm/mshyperv.h > x86/vdso: Add VCLOCK_HVCLOCK vDSO clock read method > > arch/x86/entry/vdso/vclock_gettime.c | 24 +++ > arch/x86/entry/vdso/vdso-layout.lds.S | 3 +- > arch/x86/entry/vdso/vdso2c.c | 3 ++ > arch/x86/entry/vdso/vma.c | 7 + > arch/x86/hyperv/hv_init.c | 48 + > arch/x86/include/asm/clocksource.h| 3 +- > arch/x86/include/asm/mshyperv.h | 58 > +++ > arch/x86/include/asm/vdso.h | 1 + > drivers/hv/Kconfig| 3 ++ > 9 files changed, 114 insertions(+), 36 deletions(-) > > -- > 2.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/2] x86/vdso: Add VCLOCK_HVCLOCK vDSO clock read method
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Thursday, February 9, 2017 9:08 AM
> To: Vitaly Kuznetsov <vkuzn...@redhat.com>
> Cc: x...@kernel.org; Andy Lutomirski <l...@amacapital.net>; Ingo Molnar
> <mi...@redhat.com>; H. Peter Anvin <h...@zytor.com>; KY Srinivasan
> <k...@microsoft.com>; Haiyang Zhang <haiya...@microsoft.com>; Stephen
> Hemminger <sthem...@microsoft.com>; Dexuan Cui
> <de...@microsoft.com>; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; virtualization@lists.linux-foundation.org
> Subject: Re: [PATCH 2/2] x86/vdso: Add VCLOCK_HVCLOCK vDSO clock read
> method
>
> On Thu, 9 Feb 2017, Vitaly Kuznetsov wrote:
> > +#ifdef CONFIG_HYPERV_TSCPAGE
> > +static notrace u64 vread_hvclock(int *mode)
> > +{
> > + const struct ms_hyperv_tsc_page *tsc_pg =
> > + (const struct ms_hyperv_tsc_page *)_page;
> > + u64 sequence, scale, offset, current_tick, cur_tsc;
> > +
> > + while (1) {
> > + sequence = READ_ONCE(tsc_pg->tsc_sequence);
> > + if (!sequence)
> > + break;
> > +
> > + scale = READ_ONCE(tsc_pg->tsc_scale);
> > + offset = READ_ONCE(tsc_pg->tsc_offset);
> > + rdtscll(cur_tsc);
> > +
> > + current_tick = mul_u64_u64_shr(cur_tsc, scale, 64) + offset;
> > +
> > + if (READ_ONCE(tsc_pg->tsc_sequence) == sequence)
> > + return current_tick;
>
> That sequence stuff lacks still a sensible explanation. It's fundamentally
> different from the sequence counting we do in the kernel, so documentation
> for it is really required.
The host is updating multiple fields in this shared TSC page and the sequence
number is
used to ensure that the guest sees a consistent set values published. If I
remember
correctly, Xen has a similar mechanism.
K. Y
>
> Thanks,
>
> tglx
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH 05/14] netvsc: remove no longer needed receive staging buffers
> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Wednesday, February 1, 2017 8:29 AM
> To: KY Srinivasan <k...@microsoft.com>; gre...@linuxfoundation.org
> Cc: de...@linuxdriverproject.org; virtualization@lists.linux-foundation.org;
> Stephen Hemminger <sthem...@microsoft.com>
> Subject: [PATCH 05/14] netvsc: remove no longer needed receive staging
> buffers
>
> Since commit aed8c164ca5199 ("Drivers: hv: ring_buffer: count on wrap
> around mappings") it is no longer necessary to handle ring wrapping
> by having a special receive buffer.
>
> Signed-off-by: Stephen Hemminger <sthem...@microsoft.com>
> ---
> drivers/net/hyperv/hyperv_net.h | 5 ---
> drivers/net/hyperv/netvsc.c | 83
> ++-
> drivers/net/hyperv/rndis_filter.c | 11 --
> 3 files changed, 11 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h
> b/drivers/net/hyperv/hyperv_net.h
> index 3958adade7eb..cce70ceba6d5 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -748,11 +748,6 @@ struct netvsc_device {
>
> int ring_size;
>
> - /* The primary channel callback buffer */
> - unsigned char *cb_buffer;
> - /* The sub channel callback buffer */
> - unsigned char *sub_cb_buf;
> -
> struct multi_send_data msd[VRSS_CHANNEL_MAX];
> u32 max_pkt; /* max number of pkt in one send, e.g. 8 */
> u32 pkt_align; /* alignment bytes, e.g. 8 */
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index e326e68f9f6d..7487498b663c 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -67,12 +67,6 @@ static struct netvsc_device *alloc_net_device(void)
> if (!net_device)
> return NULL;
>
> - net_device->cb_buffer = kzalloc(NETVSC_PACKET_SIZE,
> GFP_KERNEL);
> - if (!net_device->cb_buffer) {
> - kfree(net_device);
> - return NULL;
> - }
> -
> net_device->mrc[0].buf = vzalloc(NETVSC_RECVSLOT_MAX *
>sizeof(struct recv_comp_data));
>
> @@ -93,7 +87,6 @@ static void free_netvsc_device(struct netvsc_device
> *nvdev)
> for (i = 0; i < VRSS_CHANNEL_MAX; i++)
> vfree(nvdev->mrc[i].buf);
>
> - kfree(nvdev->cb_buffer);
> kfree(nvdev);
> }
>
> @@ -584,7 +577,6 @@ void netvsc_device_remove(struct hv_device
> *device)
> vmbus_close(device->channel);
>
> /* Release all resources */
> - vfree(net_device->sub_cb_buf);
> free_netvsc_device(net_device);
> }
>
> @@ -1256,16 +1248,11 @@ static void netvsc_process_raw_pkt(struct
> hv_device *device,
>
> void netvsc_channel_cb(void *context)
> {
> - int ret;
> - struct vmbus_channel *channel = (struct vmbus_channel *)context;
> + struct vmbus_channel *channel = context;
> u16 q_idx = channel->offermsg.offer.sub_channel_index;
> struct hv_device *device;
> struct netvsc_device *net_device;
> - u32 bytes_recvd;
> - u64 request_id;
> struct vmpacket_descriptor *desc;
> - unsigned char *buffer;
> - int bufferlen = NETVSC_PACKET_SIZE;
> struct net_device *ndev;
> bool need_to_commit = false;
>
> @@ -1277,65 +1264,19 @@ void netvsc_channel_cb(void *context)
> net_device = get_inbound_net_device(device);
> if (!net_device)
> return;
> +
> ndev = hv_get_drvdata(device);
> - buffer = get_per_channel_state(channel);
> -
> - do {
> - desc = get_next_pkt_raw(channel);
> - if (desc != NULL) {
> - netvsc_process_raw_pkt(device,
> -channel,
> -net_device,
> -ndev,
> -desc->trans_id,
> -desc);
> -
> - put_pkt_raw(channel, desc);
> - need_to_commit = true;
> - continue;
> - }
> - if (need_to_commit) {
> - need_to_commit = false;
> - commit_rd_index(channel);
> - }
>
> - ret = vmbus_recvpacket_raw(channel, buffer, bufferlen,
> -_recvd, _id);
> - if (ret == 0) {
> - if (bytes_recvd > 0) {
> -
RE: [Qemu-devel] [PATCH 2/2] kvm/x86: Hyper-V kvm exit
> -Original Message- > From: Denis V. Lunev [mailto:d...@openvz.org] > Sent: Monday, October 12, 2015 6:46 AM > To: Eric Blake <ebl...@redhat.com> > Cc: Gleb Natapov <g...@kernel.org>; qemu-de...@nongnu.org; > virtualization@lists.linux-foundation.org; rka...@virtuozzo.com; Paolo > Bonzini <pbonz...@redhat.com>; Andrey Smetanin > <asmeta...@virtuozzo.com>; Vitaly Kuznetsov <vkuzn...@redhat.com>; KY > Srinivasan <k...@microsoft.com> > Subject: Re: [Qemu-devel] [PATCH 2/2] kvm/x86: Hyper-V kvm exit > > On 10/12/2015 04:42 PM, Eric Blake wrote: > > On 10/09/2015 07:39 AM, Denis V. Lunev wrote: > >> From: Andrey Smetanin <asmeta...@virtuozzo.com> > >> > >> A new vcpu exit is introduced to notify the userspace of the > >> changes in Hyper-V synic configuraion triggered by guest writing to the > > s/configuraion/configuration/ > > Is 'synic' intended? Is it short for something (if so, spelling it out > > may help)? > > > > > >> +++ b/Documentation/virtual/kvm/api.txt > >> @@ -3331,6 +3331,12 @@ the userspace IOAPIC should process the EOI > and retrigger the interrupt if > >> it is still asserted. Vector is the LAPIC interrupt vector for which the > >> EOI was received. > >> > >> + /* KVM_EXIT_HYPERV */ > >> +struct kvm_hyperv_exit hyperv; > >> +Indicates that the VCPU's exits into userspace to process some tasks > > s/VCPU's/VCPU/ > > > >> +related with Hyper-V emulation. Currently used to synchronize modified > >> +Hyper-V synic state with userspace. > > Again, is 'synic' intended? Hmm, I see it throughout the patch, so it > > looks intentional, but I keep trying to read it as a typo for 'sync'. > > > this is not a typo :) Yes; the Hyper-V public functional spec has chosen this name; it stands for Synthetic Interrupt Controller. K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: Standardizing an MSR or other hypercall to get an RNG seed?
-Original Message- From: virtualization-boun...@lists.linux-foundation.org [mailto:virtualization-boun...@lists.linux-foundation.org] On Behalf Of Andy Lutomirski Sent: Wednesday, September 17, 2014 7:51 PM To: Linux Virtualization; kvm list Cc: Gleb Natapov; Paolo Bonzini; Theodore Ts'o; H. Peter Anvin Subject: Standardizing an MSR or other hypercall to get an RNG seed? Hi all- I would like to standardize on a very simple protocol by which a guest OS can obtain an RNG seed early in boot. The main design requirements are: - The interface should be very easy to use. Linux, at least, will want to use it extremely early in boot as part of kernel ASLR. This means that PCI and ACPI will not work. - It should be synchronous. We don't want to delay boot while waiting for a slow host RNG. (On Linux, at least, we have a separate interface for that: virtio-rng. I think that Windows has some support for virtio-rng as well.) - Random numbers obtained through this interface should be best-effort. We want the best quality randomness that the host can provide immediately. It seems to me that the best interface for the actual request for a random number is rdmsr. This is supported on all hypervisors and all virtualization technologies. It can return a 64 bit random number, and it is easy to rdmsr the same register more than once to get a larger random number. The main questions are what MSR index to use and how to detect the presence of the MSR. I've played with two approaches: 1. Use CPUID to detect the presence of this feature. This is very easy for KVM to implement by using a KVM-specific CPUID feature. The problem is that this will necessarily be KVM-specific, as the guest must first probe for KVM and then probe for the KVM feature. I doubt that Hyper-V, for example, wants to claim to be KVM. If we could standardize a non- hypervisor-specific CPUID feature, then this problem would go away. We would prefer a CPUID feature bit to detect this feature. 2. Detect the existence of the MSR by trying to read it and handling the #GP(0) that will occur if the MSR is not present. Linux, at least, is okay with doing this, and I have code to enable an IDT and an rdmsr fixup early enough in boot to use it for ASLR. I don't know whether other operating systems can do this, though. The major questions, then, are what enumeration mechanism should be used and what MSR index should be used. For the MSR index, we could use an MSR from the Intel range if Intel were to give explicit approval, thus guaranteeing that nothing would conflict. Or we could try to agree on an MSR index in the 0x4000-0x4fff range that is unlikely to conflict with anything. For enumeration, we could just probe the MSR if all relevant guests are okay with this or we could standardize on a CPUID-based mechanism. If we do the latter, I don't know what that mechanism would be. NB: This thread will be cc'd to Microsoft and possibly Hyper-V people shortly. I very much appreciate Jun Nakajima's help with this! Thanks, Andy Regards, K. Y -- Andy Lutomirski AMA Capital Management, LLC ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: Standardizing an MSR or other hypercall to get an RNG seed?
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Thursday, September 18, 2014 8:38 AM To: H. Peter Anvin Cc: KY Srinivasan; Linux Virtualization; kvm list; Gleb Natapov; Paolo Bonzini; Theodore Ts'o Subject: Re: Standardizing an MSR or other hypercall to get an RNG seed? On Thu, Sep 18, 2014 at 7:43 AM, H. Peter Anvin h...@zytor.com wrote: On 09/18/2014 07:40 AM, KY Srinivasan wrote: The main questions are what MSR index to use and how to detect the presence of the MSR. I've played with two approaches: 1. Use CPUID to detect the presence of this feature. This is very easy for KVM to implement by using a KVM-specific CPUID feature. The problem is that this will necessarily be KVM-specific, as the guest must first probe for KVM and then probe for the KVM feature. I doubt that Hyper-V, for example, wants to claim to be KVM. If we could standardize a non- hypervisor-specific CPUID feature, then this problem would go away. We would prefer a CPUID feature bit to detect this feature. I guess if we're introducing the concept of pan-OS MSRs we could also have pan-OS CPUID. The real issue is to get a single non-conflicting standard. Agreed. KVM currently puts 0 in 0x4000.EAX, meaning that a feature bit in Microsoft's leaf 0x4003 would probably not work well for KVM. I don't expect that Microsoft wants to start claiming to be KVM for the purpose of using a KVM-style feature bit, so, if we went the CPUID route, we would probably need something new. --Andy I am copying other Hyper-V engineers to this discussion. Regards, K. Y -hpa -- Andy Lutomirski AMA Capital Management, LLC ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: Standardizing an MSR or other hypercall to get an RNG seed?
-Original Message- From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo Bonzini Sent: Thursday, September 18, 2014 10:18 AM To: Nakajima, Jun; KY Srinivasan Cc: Mathew John; Theodore Ts'o; John Starks; kvm list; Gleb Natapov; Niels Ferguson; Andy Lutomirski; David Hepkin; H. Peter Anvin; Jake Oshins; Linux Virtualization Subject: Re: Standardizing an MSR or other hypercall to get an RNG seed? Il 18/09/2014 19:13, Nakajima, Jun ha scritto: In terms of the address for the MSR, I suggest that you choose one from the range between 4000H - 40FFH. The SDM (35.1 ARCHITECTURAL MSRS) says All existing and future processors will not implement any features using any MSR in this range. Hyper-V already defines many synthetic MSRs in this range, and I think it would be reasonable for you to pick one for this to avoid a conflict? KVM is not using any MSR in that range. However, I think it would be better to have the MSR (and perhaps CPUID) outside the hypervisor-reserved ranges, so that it becomes architecturally defined. In some sense it is similar to the HYPERVISOR CPUID feature. Yes, given that we want this to be hypervisor agnostic. K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH V2 4/4] x86: correctly detect hypervisor
-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Thursday, July 25, 2013 4:55 AM
To: t...@linutronix.de; mi...@redhat.com; h...@zytor.com; x...@kernel.org;
linux-ker...@vger.kernel.org; pbonz...@redhat.com
Cc: k...@vger.kernel.org; Jason Wang; KY Srinivasan; Haiyang Zhang; Konrad
Rzeszutek Wilk; Jeremy Fitzhardinge; Doug Covelli; Borislav Petkov; Dan Hecht;
Paul Gortmaker; Marcelo Tosatti; Gleb Natapov; Frederic Weisbecker;
de...@linuxdriverproject.org; xen-de...@lists.xensource.com;
virtualization@lists.linux-foundation.org
Subject: [PATCH V2 4/4] x86: correctly detect hypervisor
We try to handle the hypervisor compatibility mode by detecting hypervisor
through a specific order. This is not robust, since hypervisors may implement
each others features.
This patch tries to handle this situation by always choosing the last one in
the
CPUID leaves. This is done by letting .detect() returns a priority instead of
true/false and just re-using the CPUID leaf where the signature were found as
the priority (or 1 if it was found by DMI). Then we can just pick hypervisor
who
has the highest priority. Other sophisticated detection method could also be
implemented on top.
Suggested by H. Peter Anvin and Paolo Bonzini.
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: x...@kernel.org
Cc: K. Y. Srinivasan k...@microsoft.com
Cc: Haiyang Zhang haiya...@microsoft.com
Cc: Konrad Rzeszutek Wilk konrad.w...@oracle.com
Cc: Jeremy Fitzhardinge jer...@goop.org
Cc: Doug Covelli dcove...@vmware.com
Cc: Borislav Petkov b...@suse.de
Cc: Dan Hecht dhe...@vmware.com
Cc: Paul Gortmaker paul.gortma...@windriver.com
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Paolo Bonzini pbonz...@redhat.com
Cc: Frederic Weisbecker fweis...@gmail.com
Cc: linux-ker...@vger.kernel.org
Cc: de...@linuxdriverproject.org
Cc: k...@vger.kernel.org
Cc: xen-de...@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Jason Wang jasow...@redhat.com
Acked-by: K. Y. Srinivasan k...@microsoft.com
---
arch/x86/include/asm/hypervisor.h |2 +-
arch/x86/kernel/cpu/hypervisor.c | 15 +++
arch/x86/kernel/cpu/mshyperv.c| 13 -
arch/x86/kernel/cpu/vmware.c |8
arch/x86/kernel/kvm.c |6 ++
arch/x86/xen/enlighten.c |9 +++--
6 files changed, 25 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/hypervisor.h
b/arch/x86/include/asm/hypervisor.h
index 2d4b5e6..e42f758 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -33,7 +33,7 @@ struct hypervisor_x86 {
const char *name;
/* Detection routine */
- bool(*detect)(void);
+ uint32_t(*detect)(void);
/* Adjust CPU feature bits (run once per CPU) */
void(*set_cpu_features)(struct cpuinfo_x86 *);
diff --git a/arch/x86/kernel/cpu/hypervisor.c
b/arch/x86/kernel/cpu/hypervisor.c
index 8727921..36ce402 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -25,11 +25,6 @@
#include asm/processor.h
#include asm/hypervisor.h
-/*
- * Hypervisor detect order. This is specified explicitly here because
- * some hypervisors might implement compatibility modes for other
- * hypervisors and therefore need to be detected in specific sequence.
- */
static const __initconst struct hypervisor_x86 * const hypervisors[] =
{
#ifdef CONFIG_XEN_PVHVM
@@ -49,15 +44,19 @@ static inline void __init
detect_hypervisor_vendor(void)
{
const struct hypervisor_x86 *h, * const *p;
+ uint32_t pri, max_pri = 0;
for (p = hypervisors; p hypervisors + ARRAY_SIZE(hypervisors); p++) {
h = *p;
- if (h-detect()) {
+ pri = h-detect();
+ if (pri != 0 pri max_pri) {
+ max_pri = pri;
x86_hyper = h;
- printk(KERN_INFO Hypervisor detected: %s\n, h-
name);
- break;
}
}
+
+ if (max_pri)
+ printk(KERN_INFO Hypervisor detected: %s\n, x86_hyper-
name);
}
void init_hypervisor(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 8f4be53..71a39f3 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -27,20 +27,23 @@
struct ms_hyperv_info ms_hyperv;
EXPORT_SYMBOL_GPL(ms_hyperv);
-static bool __init ms_hyperv_platform(void)
+static uint32_t __init ms_hyperv_platform(void)
{
u32 eax;
u32 hyp_signature[3];
if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
- return false;
+ return 0;
cpuid(HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS
RE: [PATCH] Staging: hv: storvsc: Show the modulename in /sys/class/scsi_host/*/proc_name
-Original Message-
From: Olaf Hering [mailto:o...@aepfle.de]
Sent: Thursday, September 08, 2011 12:27 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: [PATCH] Staging: hv: storvsc: Show the modulename in
/sys/class/scsi_host/*/proc_name
mkinitrd relies on /sys/class/scsi_host/*/proc_name instead of
/sys/block/sd*/device/../../../moalias to get the scsi driver module
name.
As a fallback the sysfs driver name could be used, which does not match
the module name either ('storvsc' vs. 'hv_storvsc').
Signed-off-by: Olaf Hering o...@aepfle.de
---
drivers/staging/hv/storvsc_drv.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -1333,7 +1333,8 @@ static DEF_SCSI_QCMD(storvsc_queuecomman
/* Scsi driver */
static struct scsi_host_template scsi_driver = {
.module = THIS_MODULE,
- .name = storvsc_host_t,
+ .name = hv_storvsc,
+ .proc_name =hv_storvsc,
.bios_param = storvsc_get_chs,
.queuecommand = storvsc_queuecommand,
.eh_host_reset_handler =storvsc_host_reset_handler,
Acked-by: K. Y. Srinivasan k...@microsoft.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] Staging: hv: Integrate the time source driver with Hyper-V detection code
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Friday, September 02, 2011 4:14 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; a...@firstfloor.org; a...@linux-foundation.org; x...@kernel.org; Haiyang Zhang Subject: Re: [PATCH 1/1] Staging: hv: Integrate the time source driver with Hyper- V detection code On Tue, 2 Aug 2011, K. Y. Srinivasan wrote: The Hyper-V timesource driver is best integrated with Hyper-V detection code since: (a) Linux guests running on Hyper-V need this timesource and (b) by integrating with Hyper-V detection, we could significantly minimize the code in the timesource driver. Andrew, could you take this patch for the -mm tree. I have sent this patch out multiple times and I have not recived any response or comments. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Acked-by: Thomas Gleixner t...@linutronix.de Thanks Thomas. As you are one of the maintainers, would you be taking this patch? Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Thursday, September 01, 2011 4:31 PM To: Peter Foley Cc: KY Srinivasan; gre...@suse.de; Linux Kernel Mailing List; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V TODO file On Wed, Aug 31, 2011 at 07:11:39PM -0400, Peter Foley wrote: On Wed, 31 Aug 2011, K. Y. Srinivasan wrote: Greg, as I have been pestering you for some time now, we are very interested in exiting staging and we are willing to dedicate the necessary development resources to address whatever issues that may still be pending. So, your help in identifying what needs to be done will be greatly appreciated. To that end, I think it will be useful to update the TODO file to reflect the current state of the drivers. Let us know how we should proceed here. An issue I've come across in the hyper-v drivers is the forced modular build. You might want to see if the depends on m is still needed. Ah, good point, KY, can you try this out and see if that can be changed? If not, something is wrong and that needs to get fixed. I will check it and let you know. If there are issues, I fill fix them Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Thursday, September 01, 2011 4:31 PM To: Peter Foley Cc: KY Srinivasan; gre...@suse.de; Linux Kernel Mailing List; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V TODO file On Wed, Aug 31, 2011 at 07:11:39PM -0400, Peter Foley wrote: On Wed, 31 Aug 2011, K. Y. Srinivasan wrote: Greg, as I have been pestering you for some time now, we are very interested in exiting staging and we are willing to dedicate the necessary development resources to address whatever issues that may still be pending. So, your help in identifying what needs to be done will be greatly appreciated. To that end, I think it will be useful to update the TODO file to reflect the current state of the drivers. Let us know how we should proceed here. An issue I've come across in the hyper-v drivers is the forced modular build. You might want to see if the depends on m is still needed. Ah, good point, KY, can you try this out and see if that can be changed? If not, something is wrong and that needs to get fixed. Just verified that we can build into the kernel (not as modules) all hyper-V drivers except hv_storvsc. hv_storvsc depends upon functionality that in my build is being built as modules (SCSI etc.) and this is the reason why I could not build hv_storvsc as part of the kernel. Greg, do you want me to submit a patch to Kconfig, getting rid of the module dependency (for hyperv-v drivers). Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH] Staging: hv: vmbus: Show the modalias in /sys/bus/vmbus/devices/*/
-Original Message-
From: Olaf Hering [mailto:o...@aepfle.de]
Sent: Friday, September 02, 2011 12:26 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: [PATCH] Staging: hv: vmbus: Show the modalias in
/sys/bus/vmbus/devices/*/
Show a modalias file in /sys/bus/vmbus/devices/*/
Add a helper function to print the same content in modalias and uevent.
Signed-off-by: Olaf Hering o...@aepfle.de
---
drivers/staging/hv/vmbus_drv.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -93,6 +93,14 @@ static void get_channel_info(struct hv_d
debug_info.outbound.bytes_avail_towrite;
}
+#define VMBUS_ALIAS_LEN ((sizeof((struct hv_vmbus_device_id *)0)-guid) *
2)
+static void print_alias_name(struct hv_device *hv_dev, char *alias_name)
+{
+ int i;
+ for (i = 0; i VMBUS_ALIAS_LEN; i += 2)
+ sprintf(alias_name[i], %02x, hv_dev-dev_type.b[i/2]);
+}
+
/*
* vmbus_show_device_attr - Show the device attribute in sysfs.
*
@@ -105,6 +113,7 @@ static ssize_t vmbus_show_device_attr(st
{
struct hv_device *hv_dev = device_to_hv_device(dev);
struct hv_device_info device_info;
+ char alias_name[VMBUS_ALIAS_LEN + 1];
memset(device_info, 0, sizeof(struct hv_device_info));
@@ -148,6 +157,9 @@ static ssize_t vmbus_show_device_attr(st
device_info.chn_instance.b[13],
device_info.chn_instance.b[14],
device_info.chn_instance.b[15]);
+ } else if (!strcmp(dev_attr-attr.name, modalias)) {
+ print_alias_name(hv_dev, alias_name);
+ return sprintf(buf, vmbus:%s\n, alias_name);
} else if (!strcmp(dev_attr-attr.name, state)) {
return sprintf(buf, %d\n, device_info.chn_state);
} else if (!strcmp(dev_attr-attr.name, id)) {
@@ -204,6 +216,7 @@ static struct device_attribute vmbus_dev
__ATTR(class_id, S_IRUGO, vmbus_show_device_attr, NULL),
__ATTR(device_id, S_IRUGO, vmbus_show_device_attr, NULL),
__ATTR(monitor_id, S_IRUGO, vmbus_show_device_attr, NULL),
+ __ATTR(modalias, S_IRUGO, vmbus_show_device_attr, NULL),
__ATTR(server_monitor_pending, S_IRUGO, vmbus_show_device_attr,
NULL),
__ATTR(server_monitor_latency, S_IRUGO, vmbus_show_device_attr,
NULL),
@@ -242,12 +255,10 @@ static struct device_attribute vmbus_dev
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
struct hv_device *dev = device_to_hv_device(device);
- int i, ret;
- char alias_name[((sizeof((struct hv_vmbus_device_id *)0)-guid) + 1) *
2];
-
- for (i = 0; i ((sizeof((struct hv_vmbus_device_id *)0)-guid) * 2); i
+= 2)
- sprintf(alias_name[i], %02x, dev-dev_type.b[i/2]);
+ int ret;
+ char alias_name[VMBUS_ALIAS_LEN + 1];
+ print_alias_name(dev, alias_name);
ret = add_uevent_var(env, MODALIAS=vmbus:%s, alias_name);
return ret;
}
Acked-by: K. Y. Srinivasan k...@microsoft.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH] Staging: hv: vmbus: Show the modalias in /sys/bus/vmbus/devices/*/
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Friday, September 02, 2011 12:36 PM
To: Olaf Hering
Cc: KY Srinivasan; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org
Subject: Re: [PATCH] Staging: hv: vmbus: Show the modalias in
/sys/bus/vmbus/devices/*/
On Fri, Sep 02, 2011 at 06:25:56PM +0200, Olaf Hering wrote:
Show a modalias file in /sys/bus/vmbus/devices/*/
Add a helper function to print the same content in modalias and uevent.
Signed-off-by: Olaf Hering o...@aepfle.de
Nice idea, thanks for this, one minor nit below:
---
drivers/staging/hv/vmbus_drv.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -93,6 +93,14 @@ static void get_channel_info(struct hv_d
debug_info.outbound.bytes_avail_towrite;
}
+#define VMBUS_ALIAS_LEN ((sizeof((struct hv_vmbus_device_id *)0)-guid)
* 2)
+static void print_alias_name(struct hv_device *hv_dev, char *alias_name)
+{
+ int i;
+ for (i = 0; i VMBUS_ALIAS_LEN; i += 2)
+ sprintf(alias_name[i], %02x, hv_dev-dev_type.b[i/2]);
+}
+
/*
* vmbus_show_device_attr - Show the device attribute in sysfs.
*
@@ -105,6 +113,7 @@ static ssize_t vmbus_show_device_attr(st
{
struct hv_device *hv_dev = device_to_hv_device(dev);
struct hv_device_info device_info;
+ char alias_name[VMBUS_ALIAS_LEN + 1];
Is that too big to put on the stack? 64 bytes, right? Hm, maybe not.
Wait, hv_device_info is huge, that should be dynamic in the first place.
Olaf, not your issue, but KY, want to fix that up?
hv_device_info is about 101 bytes. I will fix this. Greg, if you are applying
Olaf's patch,
I will generate my patch on top of Olaf's. Let me know.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, September 01, 2011 4:31 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: Hyper-V TODO file
On Thu, Sep 01, 2011 at 01:27:33PM -0700, Greg KH wrote:
On Wed, Aug 31, 2011 at 03:16:51PM -0700, K. Y. Srinivasan wrote:
2) With your help, we have fixed all of the issues related to these
drivers
conforming to the Linux Device Driver model. One of the TODO work items is
audit the vmbus to verify it is working properly with the driver model.
I have a few comments about this, I'll respond in another email.
Ok, it looks a _lot_ better, but I have a few minor nits, and one larger
one:
- rename the vmbus_child_* functions to vmbus_* as there's no need to
think of children here.
- vmbus_onoffer comment is incorrect. You handle offers from lots of
other types. Or if not, am I reading the code incorrectly?
- the static hv_cb_utils array needs to go away. In the hv_utils.c
util_probe() call, properly register the channel callback, and the
same goes for the util_remove() call, unregister things there.
Note, you can use the driver_data field to determine exactly which
callback needs to be registered to make things easy. Something like
the following (pseudo code only):
static const struct hv_vmbus_device_id id_table[] = {
/* Shutdown guid */
{ VMBUS_DEVICE(0x31, 0x60, 0x0B, 0X0E, 0x13, 0x52, 0x34, 0x49,
0x81, 0x8B, 0x38, 0XD9, 0x0C, 0xED, 0x39, 0xDB),
.driver_data = shutdown_onchannelcallback },
};
util_probe(struct hv_device *dev, const struct hv_vmbus_device_id *id)
[ Yes, you will have to change the probe callback signature, but that's
fine. ]
Greg, I think if I understand you correctly, the id parameter to the probe
function
would be pointing to relevant entry in the hv_vmbus_device_id array. Since the
driver
can handle multiple ids (guids), we need to either in the driver specific probe
function or
in the bus specific probe function, figure out which of the many devices the
driver
supports is being probed. I have couple of implementation options and would
appreciate
your preference:
1) Do the guid parsing at the vmbus level parsing function. If I go this route,
the driver specific probe
function would get an extra parameter as you have described in pseudo code.
2) Do the guid parsing in the util probe function. In this case, we don't need
to change the signature for
the probe function as all the id information is available in the (util) driver.
I personally would prefer the second approach since it does not affect other
drivers (no need to change
the signature for the probe function). Let me know how you want me to proceed
here.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Friday, September 02, 2011 3:58 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: Hyper-V TODO file
On Fri, Sep 02, 2011 at 07:47:12PM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, September 01, 2011 4:31 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: Hyper-V TODO file
On Thu, Sep 01, 2011 at 01:27:33PM -0700, Greg KH wrote:
On Wed, Aug 31, 2011 at 03:16:51PM -0700, K. Y. Srinivasan wrote:
2) With your help, we have fixed all of the issues related to these
drivers
conforming to the Linux Device Driver model. One of the TODO work
items is
audit the vmbus to verify it is working properly with the driver
model.
I have a few comments about this, I'll respond in another email.
Ok, it looks a _lot_ better, but I have a few minor nits, and one larger
one:
- rename the vmbus_child_* functions to vmbus_* as there's no need to
think of children here.
- vmbus_onoffer comment is incorrect. You handle offers from lots of
other types. Or if not, am I reading the code incorrectly?
- the static hv_cb_utils array needs to go away. In the hv_utils.c
util_probe() call, properly register the channel callback, and the
same goes for the util_remove() call, unregister things there.
Note, you can use the driver_data field to determine exactly which
callback needs to be registered to make things easy. Something like
the following (pseudo code only):
static const struct hv_vmbus_device_id id_table[] = {
/* Shutdown guid */
{ VMBUS_DEVICE(0x31, 0x60, 0x0B, 0X0E, 0x13, 0x52, 0x34, 0x49,
0x81, 0x8B, 0x38, 0XD9, 0x0C, 0xED, 0x39, 0xDB),
.driver_data = shutdown_onchannelcallback },
};
util_probe(struct hv_device *dev, const struct hv_vmbus_device_id *id)
[ Yes, you will have to change the probe callback signature, but that's
fine. ]
Greg, I think if I understand you correctly, the id parameter to the probe
function
would be pointing to relevant entry in the hv_vmbus_device_id array.
Yes, just like it does for USB, PCI, etc.
Since the driver can handle multiple ids (guids), we need to either in
the driver specific probe function or in the bus specific probe
function, figure out which of the many devices the driver supports is
being probed. I have couple of implementation options and would
appreciate your preference:
1) Do the guid parsing at the vmbus level parsing function. If I go
this route, the driver specific probe function would get an extra
parameter as you have described in pseudo code.
Yes, that's the proper way to do this, as your match function already
found the proper id structure, so you have the pointer, just pass it to
the probe function callback.
2) Do the guid parsing in the util probe function. In this case, we
don't need to change the signature for the probe function as all the
id information is available in the (util) driver.
Yes, but you end up doing the matching all over again in the util
driver, which isn't nice and ripe for duplication and bugs.
I personally would prefer the second approach since it does not affect
other drivers (no need to change the signature for the probe
function). Let me know how you want me to proceed here.
As you only have 3 probe functions, it's not a big deal to change them
all :)
Ok; will do. I am glad I checked with you!
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 0000/0046] Staging: hv: Driver cleanup
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 30, 2011 1:44 PM
To: KY Srinivasan
Cc: Olaf Hering; de...@linuxdriverproject.org; gre...@suse.de; linux-
ker...@vger.kernel.org; virtualizat...@lists.osdl.org
Subject: Re: [PATCH /0046] Staging: hv: Driver cleanup
On Tue, Aug 30, 2011 at 05:22:54PM +, KY Srinivasan wrote:
-Original Message-
From: Olaf Hering [mailto:o...@aepfle.de]
Sent: Tuesday, August 30, 2011 8:49 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: [PATCH /0046] Staging: hv: Driver cleanup
On Sat, Aug 27, K. Y. Srinivasan wrote:
2) Handle all block devices using the storvsc driver. I have
modified
the implementation here based on Christoph's feedback on my
earlier
implementation.
The upgrade path from hv_blkvsc to hv_storvsc is difficult.
hv_storvsc does not provide the vmbus:hv_block modalias, so mkinitrd
does not know what module to choose when mkinitrd is called in a
previous kernel (like sles11sp1).
In a guest with both SCSI and IDE controllers the IDE disks are
discovered first, so the SCSI drives will also change their names.
Thats not a problem for data partitions because they could be mounted by
UUID or LABEL in fstab.
But its difficult to adjust /boot/grub/device.map for example because
the old IDE drives do not provide an identifier. What is the best way
to make sure the rename from hd* to sd* in such config files is done
correctly? Is it safe to assume that all drives with a class_id of
32412632-86cb-44a2-9b5c50d1417354f5 are connected to IDE ports?
The class_id is invariant as we are returning the guid of the device under
class_id.
So, if you see a IDE guid under class_id, you can be sure (a) it is an ide
device
and (b)
it is a device managed via the PV drivers.
localhost:~ # head /sys/bus/vmbus/devices/vmbus_0_{1,2,3,10,11}/class_id
== /sys/bus/vmbus/devices/vmbus_0_1/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_2/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_3/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_10/class_id ==
{ba6163d9-04a1-4d29-b60572e2ffb1dc7f}
== /sys/bus/vmbus/devices/vmbus_0_11/class_id ==
{ba6163d9-04a1-4d29-b60572e2ffb1dc7f}
In my test system, the IDE drives are now discovered twice, once by
hv_storvsc and once by libata:
This is a known (old problem). The way this was handled earlier was to have
the
modprobe rules in place to setup a dependency that would force the load of
the
hyper-v driver (blk / stor) ahead of the native driver and if the load of
the PV
driver succeeded, we would not load the native driver. In sles11 sp1, we
had a
rule for
loading blkvsc. With the merge of blkvsc and storvsc, the only change we
need
to make
is to have storvsc in the rule (instaed of blkvsc).
Why do we need a rule at all? Shouldn't the module dependancy stuff
handle the autoloading of the drivers properly from the initrd now that
the hotplug logic is hooked up properly?
What am I missing here?
On sles11 the PV drivers are used to manage the root device. Autoloading is not
the issue here; to manage the root device, we want to load the Hyper-V disk
driver
before the native Linux disk driver for the emulated IDE device.
Or is the hotplug code not working correctly?
Hotplug code is working as expected; the issue is taking control of the root
device and
preventing the same device being presented once through native drivers and once
through
Hyper-V drivers.
Regards,
K. Y
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 37/46] Staging: hv: vmbus: Check for events before messages
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 30, 2011 1:38 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 37/46] Staging: hv: vmbus: Check for events before messages On Tue, Aug 30, 2011 at 05:06:39PM +, KY Srinivasan wrote: On Sat, Aug 27, 2011 at 11:31:36AM -0700, K. Y. Srinivasan wrote: Conform to Windows specification by checking for events before messages. What specification? Care to provide a comment in the code that you are doing this in this explicit order because of some rule that the hypervisor imposes on us? I am not sure if this is documented anywhere (publicly). In talking to Windows guys, they suggested this is the order in which it is done on Windows guests and suggested I should do the same. I will see if there is some public documentation that specifies this. All you need to do is just document it in the code and I'll be happy. We don't rely on external documentation for things to be changed, in fact, there usually isn't any such thing :) Ok; will do. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Thursday, September 01, 2011 4:28 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V TODO file On Wed, Aug 31, 2011 at 03:16:51PM -0700, K. Y. Srinivasan wrote: Greg, The TODO file for Hyper-V drivers has not been updated in a while and does not reflect the current state of these drivers: 1) There are no more checkpatch warnings/errors in this code. One of the TODO work items is fix remaining checkpatch warnings and errors. Great, you can delete it. Will do. 2) With your help, we have fixed all of the issues related to these drivers conforming to the Linux Device Driver model. One of the TODO work items is audit the vmbus to verify it is working properly with the driver model. I have a few comments about this, I'll respond in another email. 3) Like all other virtualization platforms, the protocol to communicate with the host is very host specific. The ringbuffer control structures are shared between the host and the guest and so, the guest is compelled to follow the mandates of the host. Thus, it is not possible for us to merge the vmbus with other virtual buses in the system. One of the TODO work items is see if the vmbus can be merged with the other virtual busses in the kernel. Sure, you can delete it. Will do. 4) A couple of months ago, we had posted the network driver for community review. We have submitted patches (and these have been applied) to address the review comments. One of the TODO work items is audit the network driver Leave that until the network subsystem authors agree that this is all taken care of by merging the driver into their subsystem. Ok; will do. 5) Recently, we have merged the IDE and scsi drivers into a single driver that can handle both IDE and SCSI devices. These patches have been already checked in. As part of this effort, we have addressed all of the community comments on the combined storage driver. The TODO file currently has two work items on this issue: audit the block driver and audit the scsi driver. Same as above for the network driver, but you can leave just one entry now. Ok; will do. I can't apply patches right now due to the kernel.org infrastructure reworking, so you will have to wait a few days for me to apply your previous ones, but feel free to send a patch cleaning up the TODO file now. Thanks Greg. I will get the patches out soon. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V TODO file
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, September 01, 2011 4:31 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: Hyper-V TODO file
On Thu, Sep 01, 2011 at 01:27:33PM -0700, Greg KH wrote:
On Wed, Aug 31, 2011 at 03:16:51PM -0700, K. Y. Srinivasan wrote:
2) With your help, we have fixed all of the issues related to these
drivers
conforming to the Linux Device Driver model. One of the TODO work items is
audit the vmbus to verify it is working properly with the driver model.
I have a few comments about this, I'll respond in another email.
Ok, it looks a _lot_ better, but I have a few minor nits, and one larger
one:
- rename the vmbus_child_* functions to vmbus_* as there's no need to
think of children here.
Ok; will do.
- vmbus_onoffer comment is incorrect. You handle offers from lots of
other types. Or if not, am I reading the code incorrectly?
You are right; I will clean this up.
- the static hv_cb_utils array needs to go away. In the hv_utils.c
util_probe() call, properly register the channel callback, and the
same goes for the util_remove() call, unregister things there.
Note, you can use the driver_data field to determine exactly which
callback needs to be registered to make things easy. Something like
the following (pseudo code only):
static const struct hv_vmbus_device_id id_table[] = {
/* Shutdown guid */
{ VMBUS_DEVICE(0x31, 0x60, 0x0B, 0X0E, 0x13, 0x52, 0x34, 0x49,
0x81, 0x8B, 0x38, 0XD9, 0x0C, 0xED, 0x39, 0xDB),
.driver_data = shutdown_onchannelcallback },
};
util_probe(struct hv_device *dev, const struct hv_vmbus_device_id *id)
[ Yes, you will have to change the probe callback signature, but that's
fine. ]
{
void *fn(void *context);
u8 *buffer;
fn = id-driver_data;
buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
/* Hook up callback and buffer with a call to the proper vmbus
* function
*/
...
}
util_remove()
{
/* undo what you did in util_probe(), unhooking the callback and
* freeing the data */
}
Does that make any sense?
Yes; I will get these patches to you soon. I see how the data ptr can be used;
I will buy you the beer the next time we meet.
Regards,
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 41/46] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, August 29, 2011 2:09 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 41/46] Staging: hv: vmbus: Fix a bug in error handling in
vmbus_bus_init()
On Sat, Aug 27, 2011 at 11:31:40AM -0700, K. Y. Srinivasan wrote:
Fix a bug in error handling in vmbus_bus_init().
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 21 ++---
1 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 02edb11..4d1b123 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -492,7 +492,7 @@ static int vmbus_bus_init(int irq)
ret = bus_register(hv_bus);
if (ret)
- return ret;
+ goto err1;
ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
driver_name, hv_acpi_dev);
@@ -500,10 +500,7 @@ static int vmbus_bus_init(int irq)
if (ret != 0) {
pr_err(Unable to request IRQ %d\n,
irq);
-
- bus_unregister(hv_bus);
-
- return ret;
+ goto err2;
}
vector = IRQ0_VECTOR + irq;
@@ -514,16 +511,18 @@ static int vmbus_bus_init(int irq)
*/
on_each_cpu(hv_synic_init, (void *)vector, 1);
ret = vmbus_connect();
- if (ret) {
- free_irq(irq, hv_acpi_dev);
- bus_unregister(hv_bus);
- return ret;
- }
-
+ if (ret)
+ goto err3;
vmbus_request_offers();
return 0;
+
+err3: free_irq(irq, hv_acpi_dev);
+err2: bus_unregister(hv_bus);
+err1: hv_cleanup();
The traditional way to write this is:
err3:
free_irq(irq, hv_acpi_dev);
err2:
bus_unregister(hv_bus);
err1:
hv_cleanup();
Care to fix this up and resend it?
Will do.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 44/46] Staging: hv: vmbus: Fix checkpatch warnings in connection.c
-Original Message-
From: Joe Perches [mailto:j...@perches.com]
Sent: Sunday, August 28, 2011 2:57 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 44/46] Staging: hv: vmbus: Fix checkpatch warnings in
connection.c
On Sat, 2011-08-27 at 11:31 -0700, K. Y. Srinivasan wrote:
Fix checkpatch warnings in connection.c.
[]
diff --git a/drivers/staging/hv/connection.c
b/drivers/staging/hv/connection.c
[]
@@ -220,11 +220,11 @@ static void process_chn_event(u32 relid)
channel = relid2channel(relid);
spin_lock_irqsave(channel-inbound_lock, flags);
- if (channel (channel-onchannel_callback != NULL)) {
+ if (channel (channel-onchannel_callback != NULL))
Useless test for channel or bad placement for spin_lock.
channel has already been dereferenced by the spin_lock.
Thanks Joe. I will fix this up.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 37/46] Staging: hv: vmbus: Check for events before messages
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, August 29, 2011 2:05 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 37/46] Staging: hv: vmbus: Check for events before messages On Sat, Aug 27, 2011 at 11:31:36AM -0700, K. Y. Srinivasan wrote: Conform to Windows specification by checking for events before messages. What specification? Care to provide a comment in the code that you are doing this in this explicit order because of some rule that the hypervisor imposes on us? I am not sure if this is documented anywhere (publicly). In talking to Windows guys, they suggested this is the order in which it is done on Windows guests and suggested I should do the same. I will see if there is some public documentation that specifies this. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 44/46] Staging: hv: vmbus: Fix checkpatch warnings in connection.c
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, August 29, 2011 2:10 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 44/46] Staging: hv: vmbus: Fix checkpatch warnings in
connection.c
On Sat, Aug 27, 2011 at 11:31:43AM -0700, K. Y. Srinivasan wrote:
Fix checkpatch warnings in connection.c.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/connection.c | 13 +++--
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/hv/connection.c
b/drivers/staging/hv/connection.c
index ca92ca3..9e99c04 100644
--- a/drivers/staging/hv/connection.c
+++ b/drivers/staging/hv/connection.c
@@ -220,11 +220,11 @@ static void process_chn_event(u32 relid)
channel = relid2channel(relid);
spin_lock_irqsave(channel-inbound_lock, flags);
- if (channel (channel-onchannel_callback != NULL)) {
+ if (channel (channel-onchannel_callback != NULL))
channel-onchannel_callback(channel-
channel_callback_context);
- } else {
I agree with Joe here, if channel really was NULL, you just oopsed.
I'll apply this one, but please send me a follow-on one fixing this bug.
Thanks Greg. I will fix this. I got these patches out just before Hurricane
Irene
hit the east coast. While we were lucky that it was not as bad as was predicted,
we lost power and we still don't have power. I have come to a public library in
a nearby town to check my email. So, my responses will be sporadic over the
next couple of days (until we get power). I will try to address the issues you
have raised as quickly as possible.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 46/46] Staging: hv: Update the TODO file
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, August 29, 2011 2:12 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 46/46] Staging: hv: Update the TODO file On Sat, Aug 27, 2011 at 11:31:45AM -0700, K. Y. Srinivasan wrote: Update the TODO file. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com --- drivers/staging/hv/TODO | 24 1 files changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/staging/hv/TODO b/drivers/staging/hv/TODO index 582fd4a..23c74ed 100644 --- a/drivers/staging/hv/TODO +++ b/drivers/staging/hv/TODO @@ -1,14 +1,30 @@ TODO: - - fix remaining checkpatch warnings and errors - audit the vmbus to verify it is working properly with the driver model - - see if the vmbus can be merged with the other virtual busses - in the kernel + + STATUS (July 19, 2011): + All outstanding issues (known to us) with regards + to conforming to Linux Driver Model have been addressed. The TODO file is not a place to have a conversation about what is and is not completed by having status reports. Let's discuss this through email, and if we all agree, then the TODO entries can be removed. Ok; will do. K. Y greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 10/59] Staging: hv: util: Make hv_utils a vmbus device driver
-Original Message- From: Dan Carpenter [mailto:erro...@gmail.com] Sent: Tuesday, August 30, 2011 11:53 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 10/59] Staging: hv: util: Make hv_utils a vmbus device driver On Thu, Aug 25, 2011 at 09:48:36AM -0700, K. Y. Srinivasan wrote: + util_drv.driver.name = driver_name; + + return vmbus_child_driver_register(util_drv.driver); We should free the allocations if vmbus_child_driver_register() fails. Thanks Dan. I will take care of this. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 0000/0046] Staging: hv: Driver cleanup
-Original Message-
From: Olaf Hering [mailto:o...@aepfle.de]
Sent: Tuesday, August 30, 2011 8:49 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: [PATCH /0046] Staging: hv: Driver cleanup
On Sat, Aug 27, K. Y. Srinivasan wrote:
2) Handle all block devices using the storvsc driver. I have modified
the implementation here based on Christoph's feedback on my earlier
implementation.
The upgrade path from hv_blkvsc to hv_storvsc is difficult.
hv_storvsc does not provide the vmbus:hv_block modalias, so mkinitrd
does not know what module to choose when mkinitrd is called in a
previous kernel (like sles11sp1).
In a guest with both SCSI and IDE controllers the IDE disks are
discovered first, so the SCSI drives will also change their names.
Thats not a problem for data partitions because they could be mounted by
UUID or LABEL in fstab.
But its difficult to adjust /boot/grub/device.map for example because
the old IDE drives do not provide an identifier. What is the best way
to make sure the rename from hd* to sd* in such config files is done
correctly? Is it safe to assume that all drives with a class_id of
32412632-86cb-44a2-9b5c50d1417354f5 are connected to IDE ports?
The class_id is invariant as we are returning the guid of the device under
class_id.
So, if you see a IDE guid under class_id, you can be sure (a) it is an ide
device and (b)
it is a device managed via the PV drivers.
localhost:~ # head /sys/bus/vmbus/devices/vmbus_0_{1,2,3,10,11}/class_id
== /sys/bus/vmbus/devices/vmbus_0_1/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_2/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_3/class_id ==
{32412632-86cb-44a2-9b5c50d1417354f5}
== /sys/bus/vmbus/devices/vmbus_0_10/class_id ==
{ba6163d9-04a1-4d29-b60572e2ffb1dc7f}
== /sys/bus/vmbus/devices/vmbus_0_11/class_id ==
{ba6163d9-04a1-4d29-b60572e2ffb1dc7f}
In my test system, the IDE drives are now discovered twice, once by
hv_storvsc and once by libata:
This is a known (old problem). The way this was handled earlier was to have the
modprobe rules in place to setup a dependency that would force the load of the
hyper-v driver (blk / stor) ahead of the native driver and if the load of the PV
driver succeeded, we would not load the native driver. In sles11 sp1, we had a
rule for
loading blkvsc. With the merge of blkvsc and storvsc, the only change we need
to make
is to have storvsc in the rule (instaed of blkvsc).
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 41/46] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 30, 2011 10:07 AM To: Dan Carpenter Cc: KY Srinivasan; de...@linuxdriverproject.org; Haiyang Zhang; gre...@suse.de; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 41/46] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init() On Tue, Aug 30, 2011 at 01:29:49PM +0300, Dan Carpenter wrote: err3: free_irq(irq, hv_acpi_dev); err2: bus_unregister(hv_bus); err1: hv_cleanup(); Also here is an oldbie trick. You could use multiples of ten like err30, err20, and err10. That way if you can add more error handling in the middle without changing the numbering. I knew my GW-BASIC experience would come in handy one day. :) The better way to label things is based on what happens when you get there: err_irq, err_unregister, err_cleanup. Yes, that's the best way to do this, thanks for pointing it out. I will fix this up. Regards, K. Y greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 0000/0046] Staging: hv: Driver cleanup
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, August 29, 2011 2:15 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH /0046] Staging: hv: Driver cleanup On Sat, Aug 27, 2011 at 11:31:14AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers. 1) Cleanup reference counting. 2) Handle all block devices using the storvsc driver. I have modified the implementation here based on Christoph's feedback on my earlier implementation. 3) Fix bugs. 4) Accomodate some host side scsi emulation bugs. 5) In case of scsi errors off-line the device. This patch-set further reduces the size of Hyper-V drivers - the code is about 10% smaller. This reduction is mainly because we have eliminated the blkvsc driver. If my tracking is correct, I applied everything in this series except the following patches [PATCH 37/46] Staging: hv: vmbus: Check for events before messages [PATCH 41/46] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init() [PATCH 46/46] Staging: hv: Update the TODO file Please fix up patches 37 and 41 and resend, and for 46, that should be totally different as mentioned, but you might want to send a patch that at the least adds your name to the file at the bottom, as you wanted. Also note that the MAINTAINERS file should also be adjusted to match. Thanks Greg. I will try to get these patches back to you as soon as possible. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, August 24, 2011 10:40 PM To: KY Srinivasan Cc: de...@linuxdriverproject.org; Haiyang Zhang; gre...@suse.de; linux- ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h On Thu, Aug 25, 2011 at 02:27:56AM +, KY Srinivasan wrote: Since I don't have any (current) use for the driver_data pointer, I have gone ahead and cleaned up the first 74 patches without adding the driver_data. With the mushing of the patches you had proposed this is about a 60 patch series and addresses all the other comments you had in the first 74 patches. I hope I have gotten the right granularity now. If it is ok with you, I could send these out for your consideration. Please do. But if you do, do you mind if I add the driver_data pointer, so you can blame me later if no one uses it? :) Not at all! I will go ahead and send you these patches shortly. The only unresolved issue in the remaining patches (75 - 117) is the reference counting issue we have been debating. As I noted in my earlier emails on this topic, the reference counting has been there for a long time and I am reluctant get rid of that code without additional testing/analysis. So I want to propose the following options: 1) Keep the existing code and I will skip the patches that cleaned up the reference counting 2) Take the cleanup that I have implemented In either case, I would further test and analyze this code to see if (a) the race condition that is being addressed is valid and (b) if there is a different mechanism that could be used to deal with it. Given the gaping holes in the current implementation, my personal preference would be to go with the second option. Let me know what you want me to do here. Ok, that sounds acceptable, but don't add the lock to the hv_driver, or is that needed right now? Actually, last night I spent some considerable time understanding how this could be addressed differently and in a potentially simpler way. I will go ahead and implement this scheme. Hopefully, I will be able to send you these patches soon as well. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, August 25, 2011 5:00 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
On Thu, Aug 25, 2011 at 09:48:38AM -0700, K. Y. Srinivasan wrote:
Now generate appropriate uevent based on the modalias string. As part of
this,
cleanup the existing uevent code.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 60
1 files changed, 12 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index b651968..a6e7dc5 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -237,58 +237,22 @@ static struct device_attribute vmbus_device_attrs[] =
{
* This routine is invoked when a device is added or removed on the vmbus
to
* generate a uevent to udev in the userspace. The udev will then look at
its
* rule and the uevent generated here to load the appropriate driver
+ *
+ * The alias string will be of the form vmbus:guid where guid is the string
+ * representation of the device guid (each byte of the guid will be
+ * represented with two hex characters.
*/
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
struct hv_device *dev = device_to_hv_device(device);
- int ret;
-
- ret = add_uevent_var(env, VMBUS_DEVICE_CLASS_GUID={
-%02x%02x%02x%02x-%02x%02x-%02x%02x-
-%02x%02x%02x%02x%02x%02x%02x%02x},
-dev-dev_type.b[3],
-dev-dev_type.b[2],
-dev-dev_type.b[1],
-dev-dev_type.b[0],
-dev-dev_type.b[5],
-dev-dev_type.b[4],
-dev-dev_type.b[7],
-dev-dev_type.b[6],
-dev-dev_type.b[8],
-dev-dev_type.b[9],
-dev-dev_type.b[10],
-dev-dev_type.b[11],
-dev-dev_type.b[12],
-dev-dev_type.b[13],
-dev-dev_type.b[14],
-dev-dev_type.b[15]);
-
- if (ret)
- return ret;
+ int i, ret;
+ char alias_name[((sizeof(struct hv_vmbus_device_id) + 1)) * 2];
- ret = add_uevent_var(env, VMBUS_DEVICE_DEVICE_GUID={
-%02x%02x%02x%02x-%02x%02x-%02x%02x-
-%02x%02x%02x%02x%02x%02x%02x%02x},
-dev-dev_instance.b[3],
-dev-dev_instance.b[2],
-dev-dev_instance.b[1],
-dev-dev_instance.b[0],
-dev-dev_instance.b[5],
-dev-dev_instance.b[4],
-dev-dev_instance.b[7],
-dev-dev_instance.b[6],
-dev-dev_instance.b[8],
-dev-dev_instance.b[9],
-dev-dev_instance.b[10],
-dev-dev_instance.b[11],
-dev-dev_instance.b[12],
-dev-dev_instance.b[13],
-dev-dev_instance.b[14],
-dev-dev_instance.b[15]);
- if (ret)
- return ret;
+ for (i = 0; i (sizeof(struct hv_vmbus_device_id) * 2); i += 2)
+ sprintf(alias_name[i], %02x, dev-dev_type.b[i/2]);
I have to edit this to get it to work properly with the fact that I
added the driver_data field to hv_vmbus_device_id.
Arguably, one could say that this patch was always broken as you were
assuming the size of an individual field was the same size as the whole
structure, which I don't think is always the case, or at least it's not
a safe thing to assume :)
Perhaps I could have sized it based on guid size since that is what is going
to be the alias.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, August 25, 2011 5:15 PM
To: KY Srinivasan
Cc: de...@linuxdriverproject.org; Haiyang Zhang; gre...@suse.de; linux-
ker...@vger.kernel.org; virtualizat...@lists.osdl.org
Subject: Re: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
On Thu, Aug 25, 2011 at 01:59:30PM -0700, Greg KH wrote:
On Thu, Aug 25, 2011 at 09:48:38AM -0700, K. Y. Srinivasan wrote:
Now generate appropriate uevent based on the modalias string. As part of
this,
cleanup the existing uevent code.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 60
1 files changed, 12 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c
b/drivers/staging/hv/vmbus_drv.c
index b651968..a6e7dc5 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -237,58 +237,22 @@ static struct device_attribute vmbus_device_attrs[]
= {
* This routine is invoked when a device is added or removed on the vmbus
to
* generate a uevent to udev in the userspace. The udev will then look
at its
* rule and the uevent generated here to load the appropriate driver
+ *
+ * The alias string will be of the form vmbus:guid where guid is the
string
+ * representation of the device guid (each byte of the guid will be
+ * represented with two hex characters.
*/
static int vmbus_uevent(struct device *device, struct kobj_uevent_env
*env)
{
struct hv_device *dev = device_to_hv_device(device);
- int ret;
-
- ret = add_uevent_var(env, VMBUS_DEVICE_CLASS_GUID={
- %02x%02x%02x%02x-%02x%02x-%02x%02x-
- %02x%02x%02x%02x%02x%02x%02x%02x},
- dev-dev_type.b[3],
- dev-dev_type.b[2],
- dev-dev_type.b[1],
- dev-dev_type.b[0],
- dev-dev_type.b[5],
- dev-dev_type.b[4],
- dev-dev_type.b[7],
- dev-dev_type.b[6],
- dev-dev_type.b[8],
- dev-dev_type.b[9],
- dev-dev_type.b[10],
- dev-dev_type.b[11],
- dev-dev_type.b[12],
- dev-dev_type.b[13],
- dev-dev_type.b[14],
- dev-dev_type.b[15]);
-
- if (ret)
- return ret;
+ int i, ret;
+ char alias_name[((sizeof(struct hv_vmbus_device_id) + 1)) * 2];
- ret = add_uevent_var(env, VMBUS_DEVICE_DEVICE_GUID={
- %02x%02x%02x%02x-%02x%02x-%02x%02x-
- %02x%02x%02x%02x%02x%02x%02x%02x},
- dev-dev_instance.b[3],
- dev-dev_instance.b[2],
- dev-dev_instance.b[1],
- dev-dev_instance.b[0],
- dev-dev_instance.b[5],
- dev-dev_instance.b[4],
- dev-dev_instance.b[7],
- dev-dev_instance.b[6],
- dev-dev_instance.b[8],
- dev-dev_instance.b[9],
- dev-dev_instance.b[10],
- dev-dev_instance.b[11],
- dev-dev_instance.b[12],
- dev-dev_instance.b[13],
- dev-dev_instance.b[14],
- dev-dev_instance.b[15]);
- if (ret)
- return ret;
+ for (i = 0; i (sizeof(struct hv_vmbus_device_id) * 2); i += 2)
+ sprintf(alias_name[i], %02x, dev-dev_type.b[i/2]);
I have to edit this to get it to work properly with the fact that I
added the driver_data field to hv_vmbus_device_id.
You should have a copy of the patch I applied in your inbox now, can you
verify I didn't mess it up?
Greg,
I don't think I got this mail. Could you resend the mail.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 22/59] Staging: hv: vmbus: Get rid of the unused name field in struct hv_driver
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, August 25, 2011 5:28 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 22/59] Staging: hv: vmbus: Get rid of the unused name
field
in struct hv_driver
On Thu, Aug 25, 2011 at 02:24:28PM -0700, Greg KH wrote:
On Thu, Aug 25, 2011 at 09:48:48AM -0700, K. Y. Srinivasan wrote:
Get rid of the unused name field in struct hv_driver.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/hyperv.h |2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/hv/hyperv.h b/drivers/staging/hv/hyperv.h
index b8199f4..60ead66 100644
--- a/drivers/staging/hv/hyperv.h
+++ b/drivers/staging/hv/hyperv.h
@@ -802,8 +802,6 @@ struct hv_device_info {
/* Base driver object */
struct hv_driver {
- const char *name;
Wait, why is this unused? What are you going to use as your name for
the driver in sysfs then? The module name?
As much as I love seeing things deleted, I really think you need this
field.
Ah, yeah, I see why you think it's unneeded, crud like this in the
drivers:
drv-driver.name = driver_name;
No vmbus driver should ever have to touch the base struct driver on it's
own at all. Your vmbus core should properly handle telling the driver
core what the name of the driver is.
As an example, see the __pci_register_driver() function, the first thing
that code does is set the name based on the name of the larger
pci_driver structure passed to it.
Man, if you want something done right, you have to do it yourself, let
me go make these changes so you don't have to do any new work at this
point in time, hopefully your other patches will apply...
What, vmbus_child_driver_register() takes a struct driver *?
No wonder things are so messed up here, and why you got confused. Let
me pound on this for a bit to see if I can get it cleaned up to be more
sane...
Greg,
This was existing code and I realized it did not conform to the Linux
Driver Model. I fixed it in the patch set I had sent earlier (110/117).
Yesterday I only got up to 74 of the 117 patches that you commented on.
I was planning to send the rest today. If it is ok with you, please don't fix
it here
As it would break everything else. This is existing code that I am fixing in
the patch set
I will send you shortly.
Regards,
K. Y
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Thursday, August 25, 2011 6:28 PM
To: KY Srinivasan
Cc: de...@linuxdriverproject.org; Haiyang Zhang; gre...@suse.de; linux-
ker...@vger.kernel.org; virtualizat...@lists.osdl.org
Subject: Re: [PATCH 12/59] Staging: hv: vmbus: Cleanup vmbus_uevent() code
On Thu, Aug 25, 2011 at 10:20:32PM +, KY Srinivasan wrote:
I have to edit this to get it to work properly with the fact that I
added the driver_data field to hv_vmbus_device_id.
You should have a copy of the patch I applied in your inbox now, can you
verify I didn't mess it up?
Greg,
I don't think I got this mail. Could you resend the mail.
Sure, here it is:
From: K. Y. Srinivasan k...@microsoft.com
Date: Thu, 25 Aug 2011 09:48:38 -0700
Subject: [PATCH] Staging: hv: vmbus: Cleanup vmbus_uevent() code
Status: RO
Content-Length: 2871
Lines: 82
Now generate appropriate uevent based on the modalias string. As part of this,
cleanup the existing uevent code.
[gregkh - fixed code to handle driver_data portion of struct
hv_vmbus_device_id]
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Greg Kroah-Hartman gre...@suse.de
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index b651968..afb16704 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -237,58 +237,22 @@ static struct device_attribute vmbus_device_attrs[] = {
* This routine is invoked when a device is added or removed on the vmbus to
* generate a uevent to udev in the userspace. The udev will then look at its
* rule and the uevent generated here to load the appropriate driver
+ *
+ * The alias string will be of the form vmbus:guid where guid is the string
+ * representation of the device guid (each byte of the guid will be
+ * represented with two hex characters.
*/
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
struct hv_device *dev = device_to_hv_device(device);
- int ret;
-
- ret = add_uevent_var(env, VMBUS_DEVICE_CLASS_GUID={
- %02x%02x%02x%02x-%02x%02x-%02x%02x-
- %02x%02x%02x%02x%02x%02x%02x%02x},
- dev-dev_type.b[3],
- dev-dev_type.b[2],
- dev-dev_type.b[1],
- dev-dev_type.b[0],
- dev-dev_type.b[5],
- dev-dev_type.b[4],
- dev-dev_type.b[7],
- dev-dev_type.b[6],
- dev-dev_type.b[8],
- dev-dev_type.b[9],
- dev-dev_type.b[10],
- dev-dev_type.b[11],
- dev-dev_type.b[12],
- dev-dev_type.b[13],
- dev-dev_type.b[14],
- dev-dev_type.b[15]);
-
- if (ret)
- return ret;
+ int i, ret;
+ char alias_name[((sizeof((struct hv_vmbus_device_id *)0)-guid) + 1) *
2];
- ret = add_uevent_var(env, VMBUS_DEVICE_DEVICE_GUID={
- %02x%02x%02x%02x-%02x%02x-%02x%02x-
- %02x%02x%02x%02x%02x%02x%02x%02x},
- dev-dev_instance.b[3],
- dev-dev_instance.b[2],
- dev-dev_instance.b[1],
- dev-dev_instance.b[0],
- dev-dev_instance.b[5],
- dev-dev_instance.b[4],
- dev-dev_instance.b[7],
- dev-dev_instance.b[6],
- dev-dev_instance.b[8],
- dev-dev_instance.b[9],
- dev-dev_instance.b[10],
- dev-dev_instance.b[11],
- dev-dev_instance.b[12],
- dev-dev_instance.b[13],
- dev-dev_instance.b[14],
- dev-dev_instance.b[15]);
- if (ret)
- return ret;
+ for (i = 0; i ((sizeof((struct hv_vmbus_device_id *)0)-guid) * 2); i
+= 2)
+ sprintf(alias_name[i], %02x, dev-dev_type.b[i/2]);
- return 0;
+ ret = add_uevent_var(env, MODALIAS=vmbus:%s, alias_name);
+ return ret;
}
Looks good.
Thank you.
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 11:00 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code On Wed, Aug 24, 2011 at 12:38:09AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 6:50 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote: Now generate appropriate uevent based on the modalias string. As part of this, cleanup the existing uevent code. Note, you just change the user api here, did you have tools that relied on the old format? If so, they just broke :( Prior to this, I don't think autoloading worked the way it should for these modules. It didn't? How did the mouse driver get autoloaded then, through the pci/dmi tables? You are right, prior to this all drivers were loaded using pci/dmi signatures. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 10:58 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device On Wed, Aug 24, 2011 at 12:55:12AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 7:08 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote: The current mechanism for handling references in broken. Introduce a lock to protect the ext field in hv_device. Why would that lock ever be needed? How can things change to this pointer in different ways like you are thinking it could? Doesn't the reference counting in the device itself handle this properly? This is to deal with a potential race condition between the driver being unloaded and incoming traffic from the VMBUS side. The ext pointer is device specific (either pointing to a storage or a network device) and what we are protecting is the pointer being set to NULL from the unload side when we might have a racing access from the interrupt side (on the incoming vmbus traffic). I still don't think this is needed at all, the drivers should not have to worry about this. Something is wrong with the design if it is, as no other bus needs something like this, right? This notion of reference counting has been in this code ever since I have been working on this code; it just that I fixed some obvious holes/race conditions in the logic to manage the reference counting. Firstly, this is not mandated by the vmbus and is used to support the protocol between the guest and host for storage, networking etc. At least in the case of both block and networking, the client side driver code (running in the guest) Initiates book keeping messages to the host that is not known to upper level code in Linux. So for Instance, even though SCSI layer knows exactly what I/O's are pending and takes care of reference counting the structures, it does not know about messages that have been sent by the lower level storvsc driver code. If a racing unload were to happen, we could potentially dereference a NULL ext pointer from the incoming packet processing context. The cases where this can happen are even more prevalent on the networking side. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 10:58 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
manage ref_cnt
On Wed, Aug 24, 2011 at 12:58:36AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 7:10 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock
to
manage ref_cnt
On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
Now that we have a spin lock protecting access to the stor device
pointer,
use it manage the reference count as well.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/hyperv_storage.h |8
drivers/staging/hv/storvsc.c| 10 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/hv/hyperv_storage.h
b/drivers/staging/hv/hyperv_storage.h
index 53b65be..d946211 100644
--- a/drivers/staging/hv/hyperv_storage.h
+++ b/drivers/staging/hv/hyperv_storage.h
@@ -265,7 +265,7 @@ struct storvsc_device {
struct hv_device *device;
/* 0 indicates the device is being destroyed */
- atomic_t ref_count;
+ int ref_count;
Is this really needed? Can't you rely on the reference count of the
hv_device itself?
We don't have a reference count on the hv_device
Wait, why not? You shure better have a reference count on that device
if you have a pointer to it, if not, you have a bug, and that needs to
be fixed. Please reread Documentation/CodingStyle for details.
Greg,
I am confused here. The model we have is identical to other device/bus
abstractions in the kernel. For instance, neither struct pci_dev nor struct
virtio_device have an explicit reference count. However, they both embed
struct device which has the kobject structure embedded to manage
the object life cycle. This is exactly the model we have for the vmbus devices -
struct hv_device embeds struct device that embeds the struct kobject for
managing the lifecycle.
Like other bus specific devices in the kernel (pci devices, virtio devices,),
class specific vmbus devices - struct storvsc_device and struct netvsc_device
embed a pointer to the underlying struct hv_device. Furthermore, a pointer to
the class specific device structure is stashed in the struct hv_device (the ext
pointer).
This is identical what is done in the virtio blk device - look at the priv
element in struct virtio_device.
As I noted in a different email, I did not introduce this reference counting, I
just fixed some gaping
holes in the logic. The reason, I fixed the logic and kept the reference
counting is because I can
see cases where we could end up de-referencing a NULL pointer from the
interrupt side that is
trying to deliver a vmbus packet to a device that is being destroyed.
Regards,
K. Y
and this count is taken to deal with racing unloads and incoming
traffic on the channel from the host.
Is this something that all other storage drivers have to do? If not,
then you shouldn't be doing that as well.
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 10:59 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Wed, Aug 24, 2011 at 12:44:38AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 6:42 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
In preparation for implementing vmbus aliases for auto-loading
Hyper-V drivers, define vmbus specific device ID.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
include/linux/mod_devicetable.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/linux/mod_devicetable.h
b/include/linux/mod_devicetable.h
index ae28e93..5a12377 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -405,6 +405,13 @@ struct virtio_device_id {
};
#define VIRTIO_DEV_ANY_ID 0x
+/*
+ * For Hyper-V devices we use the device guid as the id.
+ */
+struct hv_vmbus_device_id {
+ __u8 guid[16];
+};
Why do you not need a driver_data pointer here? Are you sure you aren't
ever going to need it in the future? Hint, I think you will...
I am not sure I am following you here; the guid is the device ID and it is
guaranteed to remain the same. What is the driver _data pointer here
you are referring to here. While some device id have the _data pointer,
there are others that don't - for instance struct virtio_device_id. In
our case, I am not sure how I would use this private pointer.
You use it like all other drivers use it, only if needed.
Fair enough; the point is I am not sure how I would use it.
Hint, I think you need to use it in your hv_utils driver, it would
reduce the size of your code and simplify your logic.
Could you expand on this. Currently the util driver handles a bunch
services that have their own guids - and these have been included
in the idtable. How would having the pointer simplify this code.
I looked at the usage of this in PCI and it appears to be for supporting
dynamic IDs for existing drivers. I am not sure if this is applicable to the
util
driver. First, I am not sure if there will be additional services being
packaged into util driver and secondly, even if I implement this scheme
of being able to dynamically add new IDs, we still would need to add the
code to the driver to handle the new service. In fact not a whole lot of
code is shared amongst the services that are currently packaged as the
util driver.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, August 24, 2011 4:12 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Wed, Aug 24, 2011 at 04:46:11PM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 10:59 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Wed, Aug 24, 2011 at 12:44:38AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 6:42 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang
Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct
hv_vmbus_device_id
to
mod_devicetable.h
On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
In preparation for implementing vmbus aliases for auto-loading
Hyper-V drivers, define vmbus specific device ID.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
include/linux/mod_devicetable.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/linux/mod_devicetable.h
b/include/linux/mod_devicetable.h
index ae28e93..5a12377 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -405,6 +405,13 @@ struct virtio_device_id {
};
#define VIRTIO_DEV_ANY_ID 0x
+/*
+ * For Hyper-V devices we use the device guid as the id.
+ */
+struct hv_vmbus_device_id {
+ __u8 guid[16];
+};
Why do you not need a driver_data pointer here? Are you sure you
aren't
ever going to need it in the future? Hint, I think you will...
I am not sure I am following you here; the guid is the device ID and it
is
guaranteed to remain the same. What is the driver _data pointer here
you are referring to here. While some device id have the _data pointer,
there are others that don't - for instance struct virtio_device_id. In
our case, I am not sure how I would use this private pointer.
You use it like all other drivers use it, only if needed.
Fair enough; the point is I am not sure how I would use it.
Hint, I think you need to use it in your hv_utils driver, it would
reduce the size of your code and simplify your logic.
Could you expand on this. Currently the util driver handles a bunch
services that have their own guids - and these have been included
in the idtable. How would having the pointer simplify this code.
It would allow you, in your probe function, to do something different
depending on the guid that the probe function was matching on. So you
would not have to check the guid again to do that, just use the data
pointed in that void pointer and away you go.
As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
variable which uses the driver_info field to contain a quirk for the
device.
Ok; this makes sense. But I currently don't have any quirks to support!
The util driver is not even a driver in the true sense. I made it a driver and
added the probe function just to support auto-loading with the vmbus ID space
that I am trying to implement here - the probe function does nothing.
So, if it is ok with you, I will not add driver_data pointer since I will not be
using it.
I looked at the usage of this in PCI and it appears to be for supporting
dynamic IDs for existing drivers.
No, that's exactly wrong. dynamic ids play havoc with this pointer,
making some drivers not be able to handle dynamic ids because they rely
on it for some driver-specific information to be passed in it, which
dynamic ids can not handle.
Oh, have you remembered to turn off dynamic ids for these devices? Or
do you support them properly?
I don't support dynamic IDs. What would I need to do to turn it off.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, August 24, 2011 4:17 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
manage ref_cnt
On Wed, Aug 24, 2011 at 04:25:18PM +, KY Srinivasan wrote:
On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
Now that we have a spin lock protecting access to the stor device
pointer,
use it manage the reference count as well.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/hyperv_storage.h |8
drivers/staging/hv/storvsc.c| 10 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/hv/hyperv_storage.h
b/drivers/staging/hv/hyperv_storage.h
index 53b65be..d946211 100644
--- a/drivers/staging/hv/hyperv_storage.h
+++ b/drivers/staging/hv/hyperv_storage.h
@@ -265,7 +265,7 @@ struct storvsc_device {
struct hv_device *device;
/* 0 indicates the device is being destroyed */
- atomic_t ref_count;
+ int ref_count;
Is this really needed? Can't you rely on the reference count of the
hv_device itself?
We don't have a reference count on the hv_device
Wait, why not? You shure better have a reference count on that device
if you have a pointer to it, if not, you have a bug, and that needs to
be fixed. Please reread Documentation/CodingStyle for details.
Greg,
I am confused here. The model we have is identical to other device/bus
abstractions in the kernel. For instance, neither struct pci_dev nor struct
virtio_device have an explicit reference count. However, they both embed
struct device which has the kobject structure embedded to manage
the object life cycle. This is exactly the model we have for the vmbus
devices -
struct hv_device embeds struct device that embeds the struct kobject for
managing the lifecycle.
Yes, that is correct.
Like other bus specific devices in the kernel (pci devices, virtio
devices,),
class specific vmbus devices - struct storvsc_device and struct
netvsc_device
embed a pointer to the underlying struct hv_device.
And when you save that pointer, you ARE incrementing the reference count
properly, right? If not, you just caused a bug.
Why do you say that. This assignment is done in the probe function where the
driver core is invoking the driver specific probe function. In the driver
specific
probe function, I allocate class specific device state and embed the bus
specific
device pointer. So, I would think the driver core is taking the appropriate
reference
count. What I am doing is exactly what other PCI and virtio drivers are doing.
For instance,
in virtio_blk.c , virtblk_probe() function, the virtio_device pointer is
stashed away in the
virtio_blk structure in exactly the same way I am doing. I suspect the
assumption here is that
if probe succeeded, then the remove() function would be called to let the
driver cleanup the state.
Furthermore, a pointer to the class specific device structure is
stashed in the struct hv_device (the ext pointer).
This is identical what is done in the virtio blk device - look at the
priv element in struct virtio_device.
Yes, but the base structure of virtio_device does not contain a lock
that the users of that priv pointer are using to control access to data
_within_ the priv pointer, right?
True. As I noted in an earlier email, the current hyper-v driver code has logic
to deal with the race conditions I have described. It is just that the current
implementation is completely bogus - look at for instance the function
must_get_stor_device() in storvsc.c. This function is invoked in the channel
callback code path to process messages coming from the host. I added this lock
to close the window in getting the ext pointer. Clearly the lock protecting the
ext pointer must be in a structure whose persistence is guaranteed and that is
the reason
I put the lock in the struct hv_device.
That's up to the users within the priv pointer.
Now I see how you were trying to move the lock deeper as it seemed
that everyone needed it, but you just moved the lock away from the thing
that it was trying to protect, which might cause problems, and at the
least, is confusing as heck because you are mixing two different layers
here, in ways that should not be mixed.
If you really need a lock to protect some private data within the priv
pointer, then put it somewhere else,like in the priv pointer, as almost
all other subsystems do.
What is being protected is the ext pointer that points to the class specific
device state
RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
-Original Message-
From: devel-boun...@linuxdriverproject.org [mailto:devel-
boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan
Sent: Wednesday, August 24, 2011 5:51 PM
To: Greg KH
Cc: de...@linuxdriverproject.org; Haiyang Zhang; gre...@suse.de; linux-
ker...@vger.kernel.org; virtualizat...@lists.osdl.org
Subject: RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, August 24, 2011 4:12 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Wed, Aug 24, 2011 at 04:46:11PM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 10:59 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang
Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id
to
mod_devicetable.h
On Wed, Aug 24, 2011 at 12:44:38AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 6:42 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang
Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct
hv_vmbus_device_id
to
mod_devicetable.h
On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
In preparation for implementing vmbus aliases for auto-loading
Hyper-V drivers, define vmbus specific device ID.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
include/linux/mod_devicetable.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/linux/mod_devicetable.h
b/include/linux/mod_devicetable.h
index ae28e93..5a12377 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -405,6 +405,13 @@ struct virtio_device_id {
};
#define VIRTIO_DEV_ANY_ID0x
+/*
+ * For Hyper-V devices we use the device guid as the id.
+ */
+struct hv_vmbus_device_id {
+ __u8 guid[16];
+};
Why do you not need a driver_data pointer here? Are you sure you
aren't
ever going to need it in the future? Hint, I think you will...
I am not sure I am following you here; the guid is the device ID and
it is
guaranteed to remain the same. What is the driver _data pointer here
you are referring to here. While some device id have the _data
pointer,
there are others that don't - for instance struct virtio_device_id. In
our case, I am not sure how I would use this private pointer.
You use it like all other drivers use it, only if needed.
Fair enough; the point is I am not sure how I would use it.
Hint, I think you need to use it in your hv_utils driver, it would
reduce the size of your code and simplify your logic.
Could you expand on this. Currently the util driver handles a bunch
services that have their own guids - and these have been included
in the idtable. How would having the pointer simplify this code.
It would allow you, in your probe function, to do something different
depending on the guid that the probe function was matching on. So you
would not have to check the guid again to do that, just use the data
pointed in that void pointer and away you go.
As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
variable which uses the driver_info field to contain a quirk for the
device.
Ok; this makes sense. But I currently don't have any quirks to support!
The util driver is not even a driver in the true sense. I made it a driver and
added the probe function just to support auto-loading with the vmbus ID space
that I am trying to implement here - the probe function does nothing.
So, if it is ok with you, I will not add driver_data pointer since I will not
be
using it.
I looked at the usage of this in PCI and it appears to be for supporting
dynamic IDs for existing drivers.
No, that's exactly wrong. dynamic ids play havoc with this pointer,
making some drivers not be able to handle dynamic ids because they rely
on it for some driver-specific information to be passed in it, which
dynamic ids can not handle.
Oh, have you remembered to turn off dynamic ids for these devices? Or
do you support them properly
RE: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 6:50 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote: Now generate appropriate uevent based on the modalias string. As part of this, cleanup the existing uevent code. Note, you just change the user api here, did you have tools that relied on the old format? If so, they just broke :( Prior to this, I don't think autoloading worked the way it should for these modules. + for (i = 0; i (sizeof(struct hv_vmbus_device_id) * 2); i += 2) + sprintf(alias_name[i], %02x, dev-dev_type.b[i/2]); Don't we have a type for printing out a uuid already? I did not see one; could you point me to the right place. And what's with the jumping by 2 yet dividing? What am I missing here? Each byte of the uuid is represented by 2 bytes in the string; thus the magic with 2. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 6:42 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
mod_devicetable.h
On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
In preparation for implementing vmbus aliases for auto-loading
Hyper-V drivers, define vmbus specific device ID.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
include/linux/mod_devicetable.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/linux/mod_devicetable.h
b/include/linux/mod_devicetable.h
index ae28e93..5a12377 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -405,6 +405,13 @@ struct virtio_device_id {
};
#define VIRTIO_DEV_ANY_ID 0x
+/*
+ * For Hyper-V devices we use the device guid as the id.
+ */
+struct hv_vmbus_device_id {
+ __u8 guid[16];
+};
Why do you not need a driver_data pointer here? Are you sure you aren't
ever going to need it in the future? Hint, I think you will...
I am not sure I am following you here; the guid is the device ID and it is
guaranteed to remain the same. What is the driver _data pointer here
you are referring to here. While some device id have the _data pointer,
there are others that don't - for instance struct virtio_device_id. In
our case, I am not sure how I would use this private pointer.
Regards,
K. Y
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 7:08 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote: The current mechanism for handling references in broken. Introduce a lock to protect the ext field in hv_device. Why would that lock ever be needed? How can things change to this pointer in different ways like you are thinking it could? Doesn't the reference counting in the device itself handle this properly? This is to deal with a potential race condition between the driver being unloaded and incoming traffic from the VMBUS side. The ext pointer is device specific (either pointing to a storage or a network device) and what we are protecting is the pointer being set to NULL from the unload side when we might have a racing access from the interrupt side (on the incoming vmbus traffic). Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, August 23, 2011 7:10 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
manage ref_cnt
On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
Now that we have a spin lock protecting access to the stor device pointer,
use it manage the reference count as well.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
drivers/staging/hv/hyperv_storage.h |8
drivers/staging/hv/storvsc.c| 10 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/hv/hyperv_storage.h
b/drivers/staging/hv/hyperv_storage.h
index 53b65be..d946211 100644
--- a/drivers/staging/hv/hyperv_storage.h
+++ b/drivers/staging/hv/hyperv_storage.h
@@ -265,7 +265,7 @@ struct storvsc_device {
struct hv_device *device;
/* 0 indicates the device is being destroyed */
- atomic_t ref_count;
+ int ref_count;
Is this really needed? Can't you rely on the reference count of the
hv_device itself?
We don't have a reference count on the hv_device and this count is taken
to deal with racing unloads and incoming traffic on the channel from the
host.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 0000/0117] Staging: hv: Driver cleanup
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 7:11 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH /0117] Staging: hv: Driver cleanup On Fri, Jul 15, 2011 at 10:47:04AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers. Back in June I had sent two patch sets to address these issues. I have addressed the comments I got from the community on my earlier patches here: 1) Implement code for autoloading the vmbus drivers without using PCI or DMI signatures. I have implemented this based on Greg's feedback on my earlier implementation. 2) Cleanup error handling across the board and use standard Linux error codes. 3) General cleanup 4) Some bug fixes. 5) Cleanup the reference counting mess for both stor and net devices. 6) Handle all block devices using the storvsc driver. I have modified the implementation here based on Christoph's feedback on my earlier implementation. 7) Accomodate some host side scsi emulation bugs. 8) In case of scsi errors off-line the device. Care to respin this patch series based on my comments? Greg, Thank you for your comments. I will try to re-submit these patches as soon as I possibly can. If I were to get these patches re-spun and sent to you in the next couple of days, can I still make this current merge window? Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/8] Staging: hv: vmbus: Fix checkpatch warnings
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, August 23, 2011 7:17 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 4/8] Staging: hv: vmbus: Fix checkpatch warnings On Tue, Jul 19, 2011 at 11:44:21AM -0700, K. Y. Srinivasan wrote: Fix checkpatch warnings in hv.c Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com --- drivers/staging/hv/hv.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c index e733173..14e6315 100644 --- a/drivers/staging/hv/hv.c +++ b/drivers/staging/hv/hv.c @@ -111,7 +111,7 @@ static u64 do_hypercall(u64 control, void *input, void *output) u64 hv_status = 0; u64 input_address = (input) ? virt_to_phys(input) : 0; u64 output_address = (output) ? virt_to_phys(output) : 0; - volatile void *hypercall_page = hv_context.hypercall_page; + void *hypercall_page = hv_context.hypercall_page; Are you sure? This was just someone being foolish? No other reason someone tried to use volatile here? I cannot see any reason why this needs to be volatile. Regards, K. Y greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly
-Original Message- From: Sasha Levin [mailto:levinsasha...@gmail.com] Sent: Wednesday, August 17, 2011 8:48 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote: The message processing function needs to execute on the same CPU where the interrupt was taken. tasklets cannot gurantee this; so, invoke the function directly. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com --- tasklets are guaranteed to run on the same CPU as the function that scheduled them. Unless I'm missing something? I too was under this impression until I stumbled upon this comment in include/Linux/interrupt.h where I see that there is no guarantee that tasklet would run on the same CPU that it was scheduled on (look at the first listed property): /* Tasklets --- multithreaded analogue of BHs. Main feature differing them of generic softirqs: tasklet is running only on one CPU simultaneously. Main feature differing them of BHs: different tasklets may be run simultaneously on different CPUs. Properties: * If tasklet_schedule() is called, then tasklet is guaranteed to be executed on some cpu at least once after this. . . */ Given this comment here, I felt that safest thing to do would be to just not use the tasklet in this scenario. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
-Original Message-
From: Sasha Levin [mailto:levinsasha...@gmail.com]
Sent: Wednesday, August 17, 2011 9:26 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in
vmbus_bus_init()
On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
Fix a bug in error handling in vmbus_bus_init().
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
vmbus_bus_init() has a 'cleanup' label which is used just for returning,
while the cleanup logic is replicated all over the code.
Can't we just move all of it to the cleanup label?
These are in two different functions - vmbus_bus_init(), hv_acpi_init().
hv_cleanup() rolls back state established in hv_init(). I could look at
cleaning up the error handling code.
Regards,
K. Y
drivers/staging/hv/vmbus_drv.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index f20ab9a..0b0bff3 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
tasklet_init(event_dpc, vmbus_on_event, 0);
ret = bus_register(hv_bus);
- if (ret)
+ if (ret) {
+ hv_cleanup();
return ret;
+ }
ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
driver_name, hv_acpi_dev);
@@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
pr_err(Unable to request IRQ %d\n,
irq);
+ hv_cleanup();
bus_unregister(hv_bus);
return ret;
@@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
on_each_cpu(hv_synic_init, (void *)vector, 1);
ret = vmbus_connect();
if (ret) {
+ hv_cleanup();
free_irq(irq, hv_acpi_dev);
bus_unregister(hv_bus);
return ret;
--
Sasha.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V driver patches
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, August 12, 2011 5:24 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V driver patches On Fri, Aug 12, 2011 at 10:50:50AM -0700, Greg KH wrote: On Thu, Aug 11, 2011 at 05:09:45PM -0700, K. Y. Srinivasan wrote: Greg, Some weeks ago, I had sent out a fairly large patch-set for your consideration. I know you are very busy; however, I am getting a little nervous since I have not heard from you. You sent those patches after the merge window closed, which was publicly announced on the driverdevel mailing list, and the merge window for 3.2-rc1 only opened up a scant few days ago, so not hearing from me is to be expected, right? If you note, I have not merged _any_ 3.2-rc1 patches into any of my trees yet due to the above mentioned short time frame, and due to other higher priority issues (i.e. my real job and bugfixes for 3.1). Actually, looking at my past history, I don't usually start merging the patches for the next kernel release until after the -rc2 kernel is out, so I'm doing nothing unusual here at all. Off to Vancouver, Thanks Greg. See you in Vancouver! Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 5/8] Staging: hv: mousevsc: Fix checkpatch errors and warnings
-Original Message-
From: Joe Perches [mailto:j...@perches.com]
Sent: Tuesday, July 19, 2011 3:29 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 5/8] Staging: hv: mousevsc: Fix checkpatch errors and
warnings
On Tue, 2011-07-19 at 11:44 -0700, K. Y. Srinivasan wrote:
Fix checkpatch errors and warnings.
[]
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
[]
@@ -53,7 +53,7 @@ struct hv_input_dev_info {
-#pragma pack(push,1)
+#pragma pack(push, 1)
/*
* Message types in the synthetic input protocol
*/
Perhaps it's better and more consistent with
other kernel style uses to remove #pragma pack[...]
and mark the individual structs with __packed;
Good point. Currently, this driver is not functional. When the driver
is finally functional, there is a whole lot of cleanup that is needed and I will
do what you are suggesting then. For this go around, I just wanted to address
checkpatch issues with minimal change.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] Staging: hv: Integrate the time source driver with Hyper-V detection code
-Original Message-
From: K. Y. Srinivasan [mailto:k...@microsoft.com]
Sent: Friday, July 15, 2011 9:01 PM
To: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org;
t...@linutronix.de
Cc: KY Srinivasan; Haiyang Zhang
Subject: [PATCH 1/1] Staging: hv: Integrate the time source driver with
Hyper-V
detection code
The Hyper-V timesource driver is best integrated with Hyper-V detection code
since: (a) Linux guests running on Hyper-V need this timesource and (b)
by integrating with Hyper-V detection, we could significantly minimize the
code in the timesource driver.
Thomas,
A number of weeks ago you had reviewed the time Hyper-V time source driver code
and recommended that I merge it with Hyper-V detection code. That is exactly
what I
have done here. Is this what you had in mind; your feedback would be greatly
appreciated.
I had sent this patch a couple of weeks ago and got no response. As an X86
maintainer,
would you be willing to take this patch.
Regards,
K. Y
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
---
arch/x86/kernel/cpu/mshyperv.c | 24
drivers/staging/hv/Makefile|2 +-
drivers/staging/hv/hv_timesource.c | 102
3 files changed, 25 insertions(+), 103 deletions(-)
delete mode 100644 drivers/staging/hv/hv_timesource.c
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index d944bf6..c97f88d 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -11,6 +11,8 @@
*/
#include linux/types.h
+#include linux/time.h
+#include linux/clocksource.h
#include linux/module.h
#include asm/processor.h
#include asm/hypervisor.h
@@ -36,6 +38,25 @@ static bool __init ms_hyperv_platform(void)
!memcmp(Microsoft Hv, hyp_signature, 12);
}
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource hyperv_cs = {
+ .name = hyperv_clocksource,
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+};
+
static void __init ms_hyperv_init_platform(void)
{
/*
@@ -46,6 +67,9 @@ static void __init ms_hyperv_init_platform(void)
printk(KERN_INFO HyperV: features 0x%x, hints 0x%x\n,
ms_hyperv.features, ms_hyperv.hints);
+
+
+ clocksource_register_hz(hyperv_cs, NSEC_PER_SEC/100);
}
const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index bd176b1..3e0d7e6 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o
diff --git a/drivers/staging/hv/hv_timesource.c
b/drivers/staging/hv/hv_timesource.c
deleted file mode 100644
index 0efb049..000
--- a/drivers/staging/hv/hv_timesource.c
+++ /dev/null
@@ -1,102 +0,0 @@
-/*
- * A clocksource for Linux running on HyperV.
- *
- *
- * Copyright (C) 2010, Novell, Inc.
- * Author : K. Y. Srinivasan ksriniva...@novell.com
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT. See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- */
-#define pr_fmt(fmt) KBUILD_MODNAME : fmt
-
-#include linux/version.h
-#include linux/clocksource.h
-#include linux/init.h
-#include linux/module.h
-#include linux/pci.h
-#include linux/dmi.h
-#include asm/hyperv.h
-#include asm/mshyperv.h
-#include asm/hypervisor.h
-
-#define HV_CLOCK_SHIFT 22
-
-static cycle_t read_hv_clock(struct clocksource *arg)
-{
- cycle_t current_tick;
- /*
- * Read the partition counter to get the current tick
RE: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id tables in vmbus_match()
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:03 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id
tables
in vmbus_match()
On Fri, Jul 15, 2011 at 10:46:08AM -0700, K. Y. Srinivasan wrote:
+static bool is_not_null_guid(const __u8 *guid)
+{
+ int i;
+
+ for (i = 0; i (sizeof(struct hv_vmbus_device_id)); i++)
+ if (guid[i] != 0)
+ return true;
+ return false;
+}
would be nice to add uuid_{le,be}_is_nil helpers to uuid.h. I also
think simply using a memcpy might be more efficient than the hand-rolled
loop.
Having a helper function would be great. With regards to the efficiency of this
loop,
this is not a performance critical path. If people feel strongly about this, I
could
change this; perhaps as a separate patch on top of this series; I don't want to
send a large
patch-set like this again.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:05 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
Thanks, this looks much cleaner than the initial variant.
+ if (dev_is_ide) {
+ storvsc_get_ide_info(device, target, path);
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ } else {
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ }
Is using the device_info values in both branches intentional? If so
there's no need to have these assignments duplicated.
While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 088/117] Staging: hv: netvsc: Inline the code for free_net_device()
-Original Message-
From: Dan Carpenter [mailto:erro...@gmail.com]
Sent: Saturday, July 16, 2011 6:34 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 088/117] Staging: hv: netvsc: Inline the code for
free_net_device()
On Fri, Jul 15, 2011 at 10:47:16AM -0700, K. Y. Srinivasan wrote:
-static void free_net_device(struct netvsc_device *device)
-{
- WARN_ON(atomic_read(device-refcnt) != 0);
- device-dev-ext = NULL;
device-dev-ext points to device. We set it NULL here to prevent
a use after free bug. What prevents that in the new code?
- kfree(device);
-}
This function is called in only two places; a) during adding a device when we
have a failure
(netvsc_device_add()) and during device removal (netvsc_device_remove()). I
have handled
both these cases. In any event when we are freeing the containing object via
kfree(), if somebody
is accessing the ext field via freed object, we have bigger problems to deal
with.
Regards,
K. Y
regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE devices using the storvsc driver
-Original Message-
From: Sasha Levin [mailto:levinsasha...@gmail.com]
Sent: Saturday, July 16, 2011 9:02 AM
To: KY Srinivasan
Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: RE: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
On Sat, 2011-07-16 at 12:57 +, KY Srinivasan wrote:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Friday, July 15, 2011 10:05 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang
Subject: Re: [PATCH 097/117] Staging: hv: storvsc: Add code to handle IDE
devices
using the storvsc driver
Thanks, this looks much cleaner than the initial variant.
+ if (dev_is_ide) {
+ storvsc_get_ide_info(device, target, path);
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ } else {
+ host_dev-path = device_info.path_id;
+ host_dev-target = device_info.target_id;
+ }
Is using the device_info values in both branches intentional? If so
there's no need to have these assignments duplicated.
While we set the values in both the branches, the value set is different;
The IDE side encodes the bits differently and is appropriately parsed in the
function storvsc_get_ide_info().
Is think that what Christoph meant was simplifying it to:
if (dev_is_ide)
storvsc_get_ide_info(device, target, path);
host_dev-path = device_info.path_id;
host_dev-target = device_info.target_id;
Good point! It is still too early in the morning for me.
Greg, if it is ok with you, in the interest of keeping the patch
cops happy, I can submit a patch for this on top of this series.
Regards,
K. Y
--
Sasha.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Large Patch Series in Email (was Re: [PATCH 0000/0117] Staging: hv: Driver cleanup)
-Original Message- From: Michael Witten [mailto:mfwit...@gmail.com] Sent: Friday, July 15, 2011 2:26 PM To: KY Srinivasan Cc: Greg Kroah-Hartman; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Large Patch Series in Email (was Re: [PATCH /0117] Staging: hv: Driver cleanup) On Fri, 15 Jul 2011 10:47:04 -0700, K. Y. Srinivasan wrote: Subject: [PATCH /0117] Staging: hv: Driver cleanup Now, admittedly, I'm a nobody. You and I have something in common! I am sorry for flooding your mailbox. In the past Greg had indicated that he could handle large patch-sets and so, I sent out this large patch-set. Perhaps I should not have included the vger mailing list. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, July 05, 2011 11:42 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line On Wed, Jul 06, 2011 at 12:40:42AM +, KY Srinivasan wrote: diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c index 5842db8..9496abe 100644 --- a/drivers/staging/hv/blkvsc_drv.c +++ b/drivers/staging/hv/blkvsc_drv.c @@ -1027,5 +1027,6 @@ static void __exit blkvsc_exit(void) MODULE_LICENSE(GPL); MODULE_VERSION(HV_DRV_VERSION); MODULE_DESCRIPTION(Microsoft Hyper-V virtual block driver); +MODULE_ALIAS(vmbus:hv_block); No, these should be automagically generated with the MODULE_DEVICE_ID() macro that you use in the module with the GUID there, instead of this. I think you mean MODULE_DEVICE_TABLE()? Yes, sorry for the typo. I actually went down that path first adding code to file2alias.c for parsing the vmbus ID table. Given that this approach would make it impossible to support auto-loading of these drivers on many of the released kernels, Wait, what? What is a released kernel? We are working on the in-kernel patch, we don't care about older distros/releases for this work at all. Also, it doesn't make sense at all, why would the change I asked for make any difference on older distros/kernels? I understand we don't care here about older kernels and I will do what you have suggested. I just wanted to give you the rationale for choices I made: We are currently supporting older distros/kernels using these upstream bits. With the MODULE_ALIAS() approach, since I did not have to change any code outside the hv directory, this was possible. I was mostly concerned about having to make changes to code outside the hv directory and figuring out how to build and propagate these changes (file2alias.c) in older kernels. I chose to go with the MODULE_ALIAS() macro that did not need any changes outside our drivers. In both methods, the formatting of the name is bus specific since I would be writing the code to parse the table in file2alias.c. Yes, that is what is needed to be done. Granted, I have been quite unimaginative in my alias names, but I thought they were reasonably descriptive. If at all possible, for the reasons listed above, I would prefer to use the MODULE_ALIAS() macro (I could embed all or part of the guid in the alias). Let me know. Please do the correct thing and use MODULE_DEVICE_TABLE(). We have four drivers now excluding vmbus and soon we will have only three drivers with the merge of block and stor drivers. Would you still recommend I use the full guid to name these drivers. Rather than embedding the entire 128bit guid in module aliases, I was thinking of setting up a more reasonable namespace for these drivers (like what virtio has done for instance). Let me know if this is ok with you if I took that route (mapping the guid to small integers and having these integers be used in alias strings). Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, July 06, 2011 11:02 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line On Wed, Jul 06, 2011 at 02:55:12PM +, KY Srinivasan wrote: I think you mean MODULE_DEVICE_TABLE()? Yes, sorry for the typo. I actually went down that path first adding code to file2alias.c for parsing the vmbus ID table. Given that this approach would make it impossible to support auto-loading of these drivers on many of the released kernels, Wait, what? What is a released kernel? We are working on the in-kernel patch, we don't care about older distros/releases for this work at all. Also, it doesn't make sense at all, why would the change I asked for make any difference on older distros/kernels? I understand we don't care here about older kernels and I will do what you have suggested. I just wanted to give you the rationale for choices I made: We are currently supporting older distros/kernels using these upstream bits. With the MODULE_ALIAS() approach, since I did not have to change any code outside the hv directory, this was possible. I was mostly concerned about having to make changes to code outside the hv directory and figuring out how to build and propagate these changes (file2alias.c) in older kernels. You create a patch like any other patch and give it to the people who are stuck with those older kernels. It shouldn't be that hard, and it also shouldn't be something that matters for this mainline work either. I chose to go with the MODULE_ALIAS() macro that did not need any changes outside our drivers. In both methods, the formatting of the name is bus specific since I would be writing the code to parse the table in file2alias.c. Yes, that is what is needed to be done. Granted, I have been quite unimaginative in my alias names, but I thought they were reasonably descriptive. If at all possible, for the reasons listed above, I would prefer to use the MODULE_ALIAS() macro (I could embed all or part of the guid in the alias). Let me know. Please do the correct thing and use MODULE_DEVICE_TABLE(). We have four drivers now excluding vmbus and soon we will have only three drivers with the merge of block and stor drivers. Would you still recommend I use the full guid to name these drivers. Yes, why not? Rather than embedding the entire 128bit guid in module aliases, I was thinking of setting up a more reasonable namespace for these drivers (like what virtio has done for instance). Let me know if this is ok with you if I took that route (mapping the guid to small integers and having these integers be used in alias strings). What's the big deal of having a large number as an alias? Is there some constraint here that I am not aware of? It is certainly easier to deal with a small integer than with 128bit giud and there is no added benefit dealing with guids since the number of drivers we will support under vmbus will never exceed a very small number. Having said that, I will go ahead and embed full guids as you have suggested. So, the aliases for these drivers will be: vmbus:device guid. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, July 06, 2011 11:29 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line On Wed, Jul 06, 2011 at 03:17:37PM +, KY Srinivasan wrote: It is certainly easier to deal with a small integer than with 128bit giud and there is no added benefit dealing with guids since the number of drivers we will support under vmbus will never exceed a very small number. Having said that, I will go ahead and embed full guids as you have suggested. So, the aliases for these drivers will be: vmbus:device guid. Wait, no, you will never be typing MODULE_ALIAS() in your driver at all, it will be created automatically for you by the proper macro fun with the MODULE_DEVICE_TABLE() line. The same variable you use in that macro will be used to register the driver with the vmbus core. I was referring to the name that would be generated by the parsing code In file2alias.c. I have an id_table field in the struct hv_driver and this will point to the table that will setup. This table will be used in the vmbus match function as well as to generate hotplug events. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 75/77] Staging: hv: mouse: Disable auto-loading of the mouse driver
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, July 05, 2011 11:58 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 75/77] Staging: hv: mouse: Disable auto-loading of the
mouse driver
On Thu, Jun 16, 2011 at 01:17:48PM -0700, K. Y. Srinivasan wrote:
Disable auto-loading of the mouse driver.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/hv_mouse.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 1b30f26..ab5f1da 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -950,7 +950,7 @@ static void __exit mousevsc_exit(void)
MODULE_LICENSE(GPL);
MODULE_VERSION(HV_DRV_VERSION);
-MODULE_ALIAS(vmbus:hv_mouse);
+/*MODULE_ALIAS(vmbus:hv_mouse);*/
Why?
{hint, I think I know why, but you need to tell us why in the changelog
comment}
Dropped.
I will re-submit this with appropriate comments. Have you also dropped
all remaining patches submitted in this patch-set as well as the patch-set
that I had submitted subsequently.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 01/40] Staging: hv: storvsc: Do not aquire an unnecessary reference on stor_device
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, July 05, 2011 12:14 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 01/40] Staging: hv: storvsc: Do not aquire an unnecessary reference on stor_device On Wed, Jun 29, 2011 at 07:38:58AM -0700, K. Y. Srinivasan wrote: On entry into storvsc_on_io_completion() we have already acquired a reference on the stor_device; there is no need to acquire an additional reference here. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com This series doesn't apply anymore, care to redo it? Will do. K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, July 05, 2011 12:06 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 15/77] Staging: hv: blkvsc: Add the appropriate MODULE_ALIAS() line On Thu, Jun 16, 2011 at 01:16:48PM -0700, K. Y. Srinivasan wrote: Add the appropriate MODULE_ALIAS() line to support auto-loading. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/blkvsc_drv.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c index 5842db8..9496abe 100644 --- a/drivers/staging/hv/blkvsc_drv.c +++ b/drivers/staging/hv/blkvsc_drv.c @@ -1027,5 +1027,6 @@ static void __exit blkvsc_exit(void) MODULE_LICENSE(GPL); MODULE_VERSION(HV_DRV_VERSION); MODULE_DESCRIPTION(Microsoft Hyper-V virtual block driver); +MODULE_ALIAS(vmbus:hv_block); No, these should be automagically generated with the MODULE_DEVICE_ID() macro that you use in the module with the GUID there, instead of this. I think you mean MODULE_DEVICE_TABLE()? I actually went down that path first adding code to file2alias.c for parsing the vmbus ID table. Given that this approach would make it impossible to support auto-loading of these drivers on many of the released kernels, I chose to go with the MODULE_ALIAS() macro that did not need any changes outside our drivers. In both methods, the formatting of the name is bus specific since I would be writing the code to parse the table in file2alias.c. Granted, I have been quite unimaginative in my alias names, but I thought they were reasonably descriptive. If at all possible, for the reasons listed above, I would prefer to use the MODULE_ALIAS() macro (I could embed all or part of the guid in the alias). Let me know. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, July 01, 2011 4:16 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c On Thu, Jun 30, 2011 at 08:13:51PM +, KY Srinivasan wrote: Add the contents of hyperv_storage.h to storvsc_drv.c and cleanup storvsc_drv.c.n I'd at least leave the first half of the header that defines the protocol around. I only got rid of the block comment at the start of hyperv_storage.h and consolidated the include files. Nothing of substance was deleted. I meant to say keeping a separate header file for the protocol defintion might be a good idea. That way it is kept separate from the implementation details Ok; I get it. Although, a while ago as I was cleaning up the header files In preparation to exiting staging, Greg had indicated that each driver should, if possible be a single file. I merged the relevant files to comply with that directive. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, July 01, 2011 4:17 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device On Thu, Jun 30, 2011 at 09:15:54PM +, KY Srinivasan wrote: That is what I did initially. Then looking at the way we were handling scsi devices where each scsi controller configured for the guest results in an emulated HBA (scsi host) in the guest and all the block devices under a given controller are handled through this one host, I decided to mimic a similar structure - one scsi host for all the block devices configured as an IDE device. I can go back to my earlier implementation with one host per disk. Was there any downside you noticed from using one host per emulated disk? Nothing I could see from whatever testing I did. However, the guest will have Scsi hosts that the user did not configure. This is certainly the case in both approaches - just the numbers are different! K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/40] Staging: hv: Driver cleanup
-Original Message- From: Stephen Hemminger [mailto:shemmin...@vyatta.com] Sent: Friday, July 01, 2011 12:45 AM To: KY Srinivasan Cc: Christoph Hellwig; de...@linuxdriverproject.org; gre...@suse.de; linux- ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Fri, 1 Jul 2011 00:19:38 + KY Srinivasan k...@microsoft.com wrote: -Original Message- From: Stephen Hemminger [mailto:shemmin...@vyatta.com] Sent: Thursday, June 30, 2011 7:48 PM To: KY Srinivasan Cc: Christoph Hellwig; de...@linuxdriverproject.org; gre...@suse.de; linux- ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Thu, 30 Jun 2011 23:32:34 + KY Srinivasan k...@microsoft.com wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers: 1) Cleanup the reference counting mess for both stor and net devices. I really don't understand the need for reference counting on the storage side, especially now that you only have a SCSI driver. The SCSI midlayer does proper counting on it's objects (Scsi_Host, scsi_device, scsi_cmnd), so you'll get that for free given that SCSI drivers just piggyback on the midlayer lifetime rules. For now your patches should probably go in as-is, but mid-term you should be able to completely remove that code on the storage side. Greg, I am thinking of going back to my original implementation where I had one scsi host per IDE device. This will certainly simply the code. Let me know what you think. If you agree with this approach, please drop this patch-set, I will send you a new set of patches. I think there ref counting on network devices is also unneeded as long as the unregister logic handles RCU correctly. The network layer calls the driver unregister routine after all packets are gone. On the networking side, what about incoming packets that may be racing with the device destruction. The current ref counting scheme deals with that case. Not sure how HV driver tells hypervisor to stop sending packets. But the destructor is not called until after all other CPU's are done processing packets from that device. The issue I was concerned is one where, on packet reception, we need to de-reference the ext field in the struct hv_device (this is the pointer to the net device and this could happen concurrently with the guest trying to shut-down the device. In the current code we deal with this condition. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/40] Staging: hv: Driver cleanup
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, July 01, 2011 4:22 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Thu, Jun 30, 2011 at 11:28:27PM +, KY Srinivasan wrote: On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers: 1) Cleanup the reference counting mess for both stor and net devices. I really don't understand the need for reference counting on the storage side, especially now that you only have a SCSI driver. The SCSI midlayer does proper counting on it's objects (Scsi_Host, scsi_device, scsi_cmnd), so you'll get that for free given that SCSI drivers just piggyback on the midlayer lifetime rules. The reference counting allows us to properly deal with messages coming back from the host to the guest with a racing remove of the device. I am told these messages could potentially be not a response to a message sent from the guest. To deal with that the scsi subsystem has a two stage teardown for the SCSI host. First you call scsi_remove_host, at which point no new I/O to it can be started. After that you can wait for all outstanding messages, and finally you do a scsi_host_put to drop the long-term reference to it, and eventually free it. That is what I have currently implemented both on the stor as well as the net side with regards to the device specific objects hanging off of struct hv_device (the ext field). If it is ok with you, I want to do what you had suggested earlier. Keep the patches as they are with regards to reference counting for now. We can always simplify the code later. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 38/40] Staging: hv: storvsc: Fixup srb_status for INQUIRY and MODE_SENSE
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:48 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 38/40] Staging: hv: storvsc: Fixup srb_status for INQUIRY and MODE_SENSE On Wed, Jun 29, 2011 at 07:39:35AM -0700, K. Y. Srinivasan wrote: The current handler on the Windows Host does not correctly handle INQUIRY and MODE_SENSE commands with some options. Fixup srb_status in these cases since the failure is not fatal. + /* +* The current SCSI handling on the host side does +* not correctly handle: +* INQUIRY command with page code parameter set to 0x80 +* MODE_SENSE command with cmd[2] == 0x1c +* +* Setup srb status so this won't be fatal. +*/ + + if ((stor_pkt-vm_srb.cdb[0] == INQUIRY) || + (stor_pkt-vm_srb.cdb[0] == MODE_SENSE)) + vstor_packet-vm_srb.srb_status = 0; Given that the srb_status is only used for debug printks I don't quite see the point. If people explicitly turn on debugging they should see that these commands fail, shouldn't they? The reason I did this was so that I could key off on real failures indicated by srb_status == 0x4 to off-line the device. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:46 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c On Wed, Jun 29, 2011 at 07:39:31AM -0700, K. Y. Srinivasan wrote: Add the contents of hyperv_storage.h to storvsc_drv.c and cleanup storvsc_drv.c.n I'd at least leave the first half of the header that defines the protocol around. I only got rid of the block comment at the start of hyperv_storage.h and consolidated the include files. Nothing of substance was deleted. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 23/40] Staging: hv: storvsc: Introduce code to manage IDE devices using storvsc HBA
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Thursday, June 30, 2011 3:39 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 23/40] Staging: hv: storvsc: Introduce code to manage IDE
devices using storvsc HBA
+/*
+ * We want to manage the IDE devices using standard Linux SCSI drivers
+ * using the storvsc driver.
+ * Define special channels to support this.
+ */
+
+#define HV_MAX_IDE_DEVICES 4
+#define HV_IDE_BASE_CHANNEL10
+#define HV_IDE0_DEV1 HV_IDE_BASE_CHANNEL
+#define HV_IDE0_DEV2 (HV_IDE_BASE_CHANNEL + 1)
+#define HV_IDE1_DEV1 (HV_IDE_BASE_CHANNEL + 2)
+#define HV_IDE1_DEV2 (HV_IDE_BASE_CHANNEL + 3)
This at last needs a good explanation of why these devices are called
IDE if they actually aren't. I know you've explained the reason to me
before, but it should also be in the code.
These devices are configured as IDE devices for the guest. The current
emulator supports 2 IDE controllers for a total of potentially 4 devices.
I did this to support all these 4 devices under one scsi host and used the
channel information to get at the correct device in the I/O path.
So, if you go to a model with one host per device, this would not be required.
The HV_IDE1_DEVn defines don't seem to useful to me. They are just
used in one place, and doing an opencoded HV_IDE_BASE_CHANNEL +
channel_nr would seem a lot easier to understand to me.
+static struct Scsi_Host *storvsc_host;
+
+/*
+ * State to manage IDE devices that register with the storvsc driver.
+ *
+ */
+static struct hv_device *ide_devices[HV_MAX_IDE_DEVICES];
+
+static void storvsc_get_ide_info(struct hv_device *dev, int *target, int
*path)
+{
+ *target =
+ dev-dev_instance.data[5] 8 | dev-dev_instance.data[4];
+
+ *path =
+ dev-dev_instance.data[3] 24 | dev-dev_instance.data[2] 16 |
+ dev-dev_instance.data[1] 8 | dev-dev_instance.data[0];
Pretty odd formatting, I'd rather do it as:
*target =
dev-dev_instance.data[5] 8 |
dev-dev_instance.data[4];
but more importanly what does path actually stand for here? Opencoding
this into the caller and adding proper comments explaining the scheme
might be more readable.
In the blkvsc driver, the path/target info was used to properly identify the
device - (a) the device was under the first or second IDE controller and (b)
whether it is the first or second device under the controller.
Regards,
K. Y
.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/40] Staging: hv: Driver cleanup
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers: 1) Cleanup the reference counting mess for both stor and net devices. I really don't understand the need for reference counting on the storage side, especially now that you only have a SCSI driver. The SCSI midlayer does proper counting on it's objects (Scsi_Host, scsi_device, scsi_cmnd), so you'll get that for free given that SCSI drivers just piggyback on the midlayer lifetime rules. The reference counting allows us to properly deal with messages coming back from the host to the guest with a racing remove of the device. I am told these messages could potentially be not a response to a message sent from the guest. For now your patches should probably go in as-is, but mid-term you should be able to completely remove that code on the storage side. Thanks. Sure, we will always try to simplify the code. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/40] Staging: hv: Driver cleanup
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers: 1) Cleanup the reference counting mess for both stor and net devices. I really don't understand the need for reference counting on the storage side, especially now that you only have a SCSI driver. The SCSI midlayer does proper counting on it's objects (Scsi_Host, scsi_device, scsi_cmnd), so you'll get that for free given that SCSI drivers just piggyback on the midlayer lifetime rules. For now your patches should probably go in as-is, but mid-term you should be able to completely remove that code on the storage side. Greg, I am thinking of going back to my original implementation where I had one scsi host per IDE device. This will certainly simply the code. Let me know what you think. If you agree with this approach, please drop this patch-set, I will send you a new set of patches. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/40] Staging: hv: Driver cleanup
-Original Message- From: Stephen Hemminger [mailto:shemmin...@vyatta.com] Sent: Thursday, June 30, 2011 7:48 PM To: KY Srinivasan Cc: Christoph Hellwig; de...@linuxdriverproject.org; gre...@suse.de; linux- ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Thu, 30 Jun 2011 23:32:34 + KY Srinivasan k...@microsoft.com wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Thursday, June 30, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote: Further cleanup of the hv drivers: 1) Cleanup the reference counting mess for both stor and net devices. I really don't understand the need for reference counting on the storage side, especially now that you only have a SCSI driver. The SCSI midlayer does proper counting on it's objects (Scsi_Host, scsi_device, scsi_cmnd), so you'll get that for free given that SCSI drivers just piggyback on the midlayer lifetime rules. For now your patches should probably go in as-is, but mid-term you should be able to completely remove that code on the storage side. Greg, I am thinking of going back to my original implementation where I had one scsi host per IDE device. This will certainly simply the code. Let me know what you think. If you agree with this approach, please drop this patch-set, I will send you a new set of patches. I think there ref counting on network devices is also unneeded as long as the unregister logic handles RCU correctly. The network layer calls the driver unregister routine after all packets are gone. On the networking side, what about incoming packets that may be racing with the device destruction. The current ref counting scheme deals with that case. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] Staging: hv: Integrate the time source driver with Hyper-V detection code
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Wednesday, June 29, 2011 3:07 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang Subject: Re: [PATCH 1/1] Staging: hv: Integrate the time source driver with Hyper- V detection code On Wed, Jun 29, 2011 at 12:06:04PM -0700, K. Y. Srinivasan wrote: The Hyper-V timesource driver is best integrated with Hyper-V detection code since: (a) Linux guests running on Hyper-V need this timesource and (b) by integrating with Hyper-V detection, we could minimize the code in the timesource driver. Greg, you had helped me initially in getting the Hyper-V detection code into the kernel (mshyperv.c). I am hoping you can take this patch. Nope, it needs to go through the x86 maintainers, please resend it to them. The scripts/get_maintainer.pl file should have told you this, right? Thanks Greg. I have done what you have recommended. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/3] Staging: hv: netvsc: Fix a bug in accounting transmit slots
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Thursday, June 09, 2011 1:05 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 1/3] Staging: hv: netvsc: Fix a bug in accounting transmit slots On Mon, Jun 06, 2011 at 03:57:35PM -0700, K. Y. Srinivasan wrote: I only got 2 of these three patches, what happened to the third one? Care to resend all 3 of these to ensure that I get them all? There really were only 2 patches I wanted to send. If you want I could reformat and send only the 2 I wanted to send. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Monday, June 06, 2011 7:13 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading On Mon, Jun 06, 2011 at 03:49:36PM -0700, K. Y. Srinivasan wrote: To support auto-loading the storvsc driver, add a DMI signature. The storvsc driver is not a DMI driver, but a vmbus driver. As such it should have a vmbus table that is used for autoloading, not a dmi one. A while ago, Greg introduced DMI signatures to these drivers to support auto-loading. This signature is not used for anything else. For some reason, this storvsc driver was missing this signature. I added it again to only support auto-loading. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event()
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Monday, June 06, 2011 7:15 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event() On Mon, Jun 06, 2011 at 03:49:48PM -0700, K. Y. Srinivasan wrote: Now, get rid of the unused wrapper - vmbus_onchannel_event(). I'd merge this into the previous patch. In general your patch split seem a little too fine grained to me in general. When you remove a wrapper you can inline it into the callsite directly, if you clean up a function directly inlining it into the helper is fine, etc. I agree with you that some of these patches are too fine grained; but I thought that was what was expected - one change per patch. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, June 07, 2011 2:59 PM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event() On Tue, Jun 07, 2011 at 02:59:32PM +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Monday, June 06, 2011 7:15 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event() On Mon, Jun 06, 2011 at 03:49:48PM -0700, K. Y. Srinivasan wrote: Now, get rid of the unused wrapper - vmbus_onchannel_event(). I'd merge this into the previous patch. In general your patch split seem a little too fine grained to me in general. When you remove a wrapper you can inline it into the callsite directly, if you clean up a function directly inlining it into the helper is fine, etc. I agree with you that some of these patches are too fine grained; but I thought that was what was expected - one change per patch. Yes, but don't take it to an extreme, like you have done here :) Will do in the future. Should I re-spin any of the patches in this set to make them less fine-grained. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, June 07, 2011 4:45 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers() On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote: Increase the timeout value in vmbus_request_offers(). Why? What does this solve/fix/prevent? On some very loaded systems, when we tested the 1 second timeout that we had prior to this patch was insufficient. So I bumped it to 5 seconds. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, June 07, 2011 5:28 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers() On Tue, Jun 07, 2011 at 09:20:16PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, June 07, 2011 4:45 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers() On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote: Increase the timeout value in vmbus_request_offers(). Why? What does this solve/fix/prevent? On some very loaded systems, when we tested the 1 second timeout that we had prior to this patch was insufficient. So I bumped it to 5 seconds. Ok, then this should be something that goes into 3.0 and older kernels, right? If so, please say so, with this type of description, when you submit it. Remember, don't say only what you did in the changelog comment, but why you did it if it's not obvious (and it wasn't here.) Care to resend this one with this information? Will do. Thanks, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, June 07, 2011 2:58 PM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading On Tue, Jun 07, 2011 at 02:54:25PM +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Monday, June 06, 2011 7:13 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading On Mon, Jun 06, 2011 at 03:49:36PM -0700, K. Y. Srinivasan wrote: To support auto-loading the storvsc driver, add a DMI signature. The storvsc driver is not a DMI driver, but a vmbus driver. As such it should have a vmbus table that is used for autoloading, not a dmi one. A while ago, Greg introduced DMI signatures to these drivers to support auto-loading. This signature is not used for anything else. For some reason, this storvsc driver was missing this signature. I added it again to only support auto-loading. Yes, that was there to solve the original problem of autoloading the driver. But Christoph is correct here, the vmbus needs a way to autoload its own drivers based on the GUID signatures of the devices it finds on the hyperv bus. So the correct thing to do in the long-run is to implement this (which is one of the things the bus needs to do before it can move out of staging). I'll take this patch as-is for now though, as it does solve the immediate issue. Thanks Greg. Can you give me some pointers here as to what needs to be done here. Looking at the code for virtio, it looks like I would need to add stuff to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will you accept patches for these files to support an ID space for Hyper-V vmbus drivers. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, June 07, 2011 6:25 PM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading On Tue, Jun 07, 2011 at 10:19:06PM +, KY Srinivasan wrote: Thanks Greg. Can you give me some pointers here as to what needs to be done here. Looking at the code for virtio, it looks like I would need to add stuff to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will you accept patches for these files to support an ID space for Hyper-V vmbus drivers. Yes, I will take patches for those files to implement this. Along with those files, you also need a way to tell userspace that new UIDS are found before the individual drivers are loaded. Hopefully you can do that now, but if not, it will need to be implemented. Could you elaborate on what user level changes I will have to do. Thanks, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Sunday, May 22, 2011 7:00 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver I see maintainers for each of the clocksource drivers and I see John Stultz and Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign- off on the Hyper-V clocksource. just send it to both of the with linux-kernel in Cc, and either of them will probably put it in. John, Thomas, I am working on getting Hyper-V drivers (drivers/staging/hv/*) out of staging. I would like to request you to look at the Hyper-V timesource driver: drivers/staging/hv/hv_timesource.c. The supporting code for this driver is already part of the base kernel. Let me know if this driver is ready to exit staging. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Monday, May 23, 2011 9:52 AM To: KY Srinivasan Cc: Christoph Hellwig; johns...@us.ibm.com; gre...@suse.de; linux- ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: RE: vmbus driver On Mon, 23 May 2011, KY Srinivasan wrote: I am working on getting Hyper-V drivers (drivers/staging/hv/*) out of staging. I would like to request you to look at the Hyper-V timesource driver: drivers/staging/hv/hv_timesource.c. The supporting code for this driver is already part of the base kernel. Let me know if this driver is ready to exit staging. Can you please send a patch against drivers/clocksource (the staging part is uninteresting for review). Will do. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Sunday, May 22, 2011 7:00 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver I see maintainers for each of the clocksource drivers and I see John Stultz and Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign- off on the Hyper-V clocksource. just send it to both of the with linux-kernel in Cc, and either of them will probably put it in. Will do. Thanks, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, May 20, 2011 8:27 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver On Thu, May 19, 2011 at 03:06:25PM -0700, K. Y. Srinivasan wrote: A few days ago you applied all the outstanding patches for the Hyper-V drivers. With these patches, I have addressed all of the known review comments for the vmbus driver (and a lot of comments/issues in other drivers as well). I am still hoping I can address whatever other issues/comments there might be with the intention to get the vmbus driver out of staging in the current window. What is your sense in terms of how feasible this is. From my side, I can assure you that I will address all legitimate issues in a very timely manner and this will not be dependent upon the location of the drivers (staging or outside staging). Looking forward to hearing from you. There's no point in merging it without a user. Make sure either the network or storage driver is in a good enough shape to move with it, to make sure the APIs it exports are actually sanely usable. Well, the util driver that implements a range of other services such as KVP, time synch, heartbeat etc. is also a client of the vmbus driver (perhaps not in the same way as the storage and network drivers). I was hoping to move the util driver out of staging along with the vmbus driver. On a different note, thanks to the feedback I got from you, Greg and others, both storage and network drivers are in a much better shape than they ever were. I will continue to cleanup the storage drivers and I would greatly appreciate your feedback and review. On the other hand the HV clocksource looks mostly mergeable and doesn't depend on vmbus. Send a patch to add it to drivers/clocksource to the maintainer and it should be mergeable with very little remaining cleanup. Agreed, now that the merge window is closed, I will have to wait for a few weeks. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Friday, May 20, 2011 9:05 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver On Thu, May 19, 2011 at 03:06:25PM -0700, K. Y. Srinivasan wrote: Greg, A few days ago you applied all the outstanding patches for the Hyper-V drivers. With these patches, I have addressed all of the known review comments for the vmbus driver (and a lot of comments/issues in other drivers as well). I am still hoping I can address whatever other issues/comments there might be with the intention to get the vmbus driver out of staging in the current window. What is your sense in terms of how feasible this is. From my side, I can assure you that I will address all legitimate issues in a very timely manner and this will not be dependent upon the location of the drivers (staging or outside staging). Looking forward to hearing from you. The merge window is closed now, and I'm on the road in asia for about 3 weeks, so doing this, at this point in the development cycle, is going to be hard. I'll go review the bus code again after the code is all merged with Linus, which should take a week or so depending on my schedule, and let you know what's left to do (I think there still is something wierd with the way the hv_driver is structured, but I could be wrong.) Thanks Greg. I look forward to your feedback. In the mean time, I'm sure the block and network driver still need a lot of work, and merging the bus code doesn't make much sense without them as a user as that is what people really want to use, so you can continue to work on them. I will continue to cleanup the block and network driver code. As you know the util driver is also a client of the vmbus driver (as far as the communication with the host goes). So, it may still make sense to plan for getting the vmbus driver out of staging along with the util and the timesource driver. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, May 20, 2011 8:27 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver On Thu, May 19, 2011 at 03:06:25PM -0700, K. Y. Srinivasan wrote: A few days ago you applied all the outstanding patches for the Hyper-V drivers. With these patches, I have addressed all of the known review comments for the vmbus driver (and a lot of comments/issues in other drivers as well). I am still hoping I can address whatever other issues/comments there might be with the intention to get the vmbus driver out of staging in the current window. What is your sense in terms of how feasible this is. From my side, I can assure you that I will address all legitimate issues in a very timely manner and this will not be dependent upon the location of the drivers (staging or outside staging). Looking forward to hearing from you. There's no point in merging it without a user. Make sure either the network or storage driver is in a good enough shape to move with it, to make sure the APIs it exports are actually sanely usable. On the other hand the HV clocksource looks mostly mergeable and doesn't depend on vmbus. Send a patch to add it to drivers/clocksource to the maintainer and it should be mergeable with very little remaining cleanup. I see maintainers for each of the clocksource drivers and I see John Stultz and Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign-off on the Hyper-V clocksource. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Friday, May 20, 2011 9:22 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: vmbus driver On Fri, May 20, 2011 at 01:12:32PM +, KY Srinivasan wrote: Well, the util driver that implements a range of other services such as KVP, time synch, heartbeat etc. is also a client of the vmbus driver (perhaps not in the The KVP driver is a different module as far as I can see. But it really needs a lot of work, as no one should use the ugly connector interface for new code. The closest equivalent is gennetlink, but I'd like to understand what it's actually supposed to do in practice. Chris, I wrote the KVP component of the util driver less than a year ago and This code was reviewed on this list before it was accepted. The KVP (Key Value Pair) functionality supports host based queries on the guest. The data gathering in the guest is done in user-mode and the kernel component of KVP is used to communicate with the host. I am using the connector interface to support communication between the kernel component and the user-mode daemon. The KVP functionality is needed to integrate with the Microsoft management stack on the host. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, May 17, 2011 6:32 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/2] Connector: Correctly set the error code in case of success when dispatching receive callbacks On Tue, May 17, 2011 at 03:25:38PM -0700, K. Y. Srinivasan wrote: The recent changes to the connector code introduced this bug where even when a callback was invoked, we would return an error resulting in double freeing of the skb. This patch fixes this bug. By recent what do you mean? Is this something that is in the .39 kernel? Do you have a git commit id that caused this? Or is this due to changes that are queued up for the .40 merge window? I just happened to test KVP functionality yesterday on latest bits from your tree (NEXT) and discovered that whenever something was sent to the kernel from userland on a connector channel, the system would hang. The following commit appears to be where this bug was introduced: commit 04f482faf50535229a5a5c8d629cf963899f857c Author: Patrick McHardy ka...@trash.net Date: Mon Mar 28 08:39:36 2011 + Since prior to this commit I don't see this problem. In any event the current code is clearly broken. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 115/206] Staging: hv: Use completion abstraction in struct netvsc_device
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Tuesday, May 10, 2011 3:07 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 115/206] Staging: hv: Use completion abstraction in struct netvsc_device - net_device-wait_condition = 0; ret = vmbus_sendpacket(device-channel, init_packet, sizeof(struct nvsp_message), (unsigned long)init_packet, @@ -272,10 +272,8 @@ static int netvsc_init_recv_buf(struct hv_device *device) goto cleanup; } - wait_event_timeout(net_device-channel_init_wait, - net_device-wait_condition, - msecs_to_jiffies(1000)); - BUG_ON(net_device-wait_condition == 0); + t = wait_for_completion_timeout(net_device-channel_init_wait, HZ); + BUG_ON(t == 0); I don't think you want a BUG_ON here, but rather fail the initialization. This is existing code and all I did was change the synchronization primitive used. Back in Feb. when this was done (prior to then), the wait was indefinite and one of the comments that was longstanding was that the wait had to be bounded and so these timed waits were introduced. When this was done, in some cases, there was no easy way to roll-back state if we did not get the response within our expected window of time. This is one such instance where the guest is handing over the receive buffers (guest physical addresses) to the host. There was a discussion on this very topic on this mailing list and the consensus was to leave the BUG_ON() call in cases where we could not reasonably recover. Also I think you really should add synchronous versions of vmbus_sendpacket*, to the vmbus core so that all the synchronous waiting can be consolidated into a few helpers instead of needing to opencode it everywhere. True; but having a non-blocking interface at the lowest level gives you the most flexibility. In addition to this non-blocking interface, we may want to look at potentially a blocking interface. In the current code, the blocking primitive that is embedded in a higher level abstraction is used for a variety of situations including the initial handshake which is what can be addressed with a synchronous API at the lowest level. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Tuesday, May 10, 2011 1:24 AM To: KY Srinivasan Cc: Christoph Hellwig; Greg KH; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: various vmbus review comments On Mon, May 09, 2011 at 02:56:52PM +, KY Srinivasan wrote: I will address this. Greg had a concern about module reference counting and looking at the current code, it did not appear to be an issue. The change you are suggesting will not affect the vmbus core which is what I want to focus on. I will however, fix this issue in the current round of patches I will send out this week. It very clearly affects the interface between the core and the functional drivers. Trying to submit the core without making sure the interface is exports works properly is not an overly good idea. I must be missing something here. As I look at the block driver (and this is indicative of other drivers as well); the exit routine - blkvsc_drv_exit, first iterates through all the devices it manages and invokes device_unregister() on each of the devices and then invokes vmbus_child_driver_unregister() which is just a wrapper on driver_unregister(). So, if I understand you correctly, you want the devices to persist even if there is no driver bound to them. So, if I eliminated the code that iterates over the devices and unregisters them, that should fix the problem and I can do this without changing the vmbus core interfaces. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Sunday, May 08, 2011 11:05 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: various vmbus review comments On Mon, May 09, 2011 at 01:46:56AM +, KY Srinivasan wrote: - the instances of hv_driver structures need to be static and not programatically defined, like all other USB and PCI drivers are handled. Done. You had expressed some concern that this would expose some issue with the core vmbus driver (which is what I want to concentrate on this go around). I have done this for both the block driver and the mouse driver and I am pretty sure I can do the same with the network driver. I have not currently done this for the network driver, since the number of patches I have to submit is already very large. Ok, but it shouldn't be a major change to the code, right? Right, it is not. I will submit code for the net driver after I am done with this patch-set. - module reference counting. Are you sure you got it all right for the individual modules that attach to the bus? I don't see any reference counting happening, is that correct? I have already exchanged an email with you on this. To summarize, it does not look like there is a problem - fix the sparse warnings. Mostly done; most of the errors are in the base kernel coming out of the macro page_to_pfn() - fix the use of volatile in the ring buffer code. It should not be needed and if you are relying on it, then the code is wrong. You are right; all accesses were already serialized with a spin lock and the Volatile attribute was unnecessary. - fix the namespace on the ringbuffer code to show that it really is only for the hyperv bus code internally. Done. That should be enough for at least one more set of patches for you all to work on :) Greg, I have had so much fun cleaning up these drivers that I lost track of the patch count. I have addressed all the issues you have raised in addition to some other cleanup that I was doing since about a week. As I look at the patch-set, I have little over 200 patches. If it is ok with you, I would like to send them as a single set. Let me know what you prefer. I need to circulate these patches internally before I can send them out. I should be able to send these out early next week. A single set is fine, if that's what you want to do, I can handle that amount as long as they all build all along the way and don't break anything. Thanks Greg. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Monday, May 09, 2011 10:34 AM To: KY Srinivasan Cc: Greg KH; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: various vmbus review comments On Fri, May 06, 2011 at 01:10:38PM +, KY Srinivasan wrote: I audited the block and the net drivers. As part of their exit routine, they invoke vmbus_child_driver_unregister() after properly cleaning up all the devices they are managing. Do you still see an issue with regards to module reference counting. Which is not the correct thing to do as explained in my last round of reviews. Take a look at the PCI code - the functional driver only does a foo_untegister_driver (which maps almost directly to driver_unregister), which then causes the device core to unbind the devices. The function driver must never call device_unregister directly as the device continues to exist even if no driver is bound to it. I will address this. Greg had a concern about module reference counting and looking at the current code, it did not appear to be an issue. The change you are suggesting will not affect the vmbus core which is what I want to focus on. I will however, fix this issue in the current round of patches I will send out this week. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 077/206] Staging: hv: Get rid of call to cleanup()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, May 09, 2011 6:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 077/206] Staging: hv: Get rid of call to cleanup() On Mon, May 09, 2011 at 02:55:59PM -0700, K. Y. Srinivasan wrote: cleanup() is an empty function; get rid of it. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/netvsc_drv.c |4 Just picking a random patch out of this series (which, I only received the first 132, not all 206). You need to specify a better changelog entry as you didn't say what code you are changing in the Subject:. How about: Staging: hv: netvsc_drv: Get rid of call to cleanup() Otherwise I would think (and I did) that you got rid of a global function called cleanup() in all of the hv code. Care to fix the names of your patches? Good point; I will fix up the names. Note, I wouldn't recommend sending that many patches to lkml, it's not needed, just stick to the de...@linuxdriverproject.org list as I really doubt anyone else cares about this code at the moment. Agreed; I will do that. Shall I go ahead and fix the names and send the patches just to you or do you want me to send them to the devel list as well. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, May 03, 2011 4:47 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: various vmbus review comments I just took a quick look at the vmbus code, and have the following comments: - why is count_hv_channel() even a function? Done; I got rid of this function - your .h files need to be consolidated and renamed. You will need a single hyperv.h file for include/linux/ that will contain some of what the vmbus*.h files have in it, but not all. Please merge things together to have: - include/linux/hyperv.h What is needed to build the drivers that attach to the bus - drivers/staging/hv/hyperv.h The local .h file will have the vmbus*.h remaining stuff that is only needed by the hv_vmbus.ko module to be build. Done; as I had informed you in an earlier mail, in addition to the two header files you have mentioned, I have also created driver specific header files for block and network drivers. - the instances of hv_driver structures need to be static and not programatically defined, like all other USB and PCI drivers are handled. Done. You had expressed some concern that this would expose some issue with the core vmbus driver (which is what I want to concentrate on this go around). I have done this for both the block driver and the mouse driver and I am pretty sure I can do the same with the network driver. I have not currently done this for the network driver, since the number of patches I have to submit is already very large. - module reference counting. Are you sure you got it all right for the individual modules that attach to the bus? I don't see any reference counting happening, is that correct? I have already exchanged an email with you on this. To summarize, it does not look like there is a problem - fix the sparse warnings. Mostly done; most of the errors are in the base kernel coming out of the macro page_to_pfn() - fix the use of volatile in the ring buffer code. It should not be needed and if you are relying on it, then the code is wrong. You are right; all accesses were already serialized with a spin lock and the Volatile attribute was unnecessary. - fix the namespace on the ringbuffer code to show that it really is only for the hyperv bus code internally. Done. That should be enough for at least one more set of patches for you all to work on :) Greg, I have had so much fun cleaning up these drivers that I lost track of the patch count. I have addressed all the issues you have raised in addition to some other cleanup that I was doing since about a week. As I look at the patch-set, I have little over 200 patches. If it is ok with you, I would like to send them as a single set. Let me know what you prefer. I need to circulate these patches internally before I can send them out. I should be able to send these out early next week. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Friday, May 06, 2011 11:00 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org
Subject: Re: various vmbus review comments
On Fri, May 06, 2011 at 01:10:38PM +, KY Srinivasan wrote:
No, I am referring to the module reference counting of the bus drivers
that register with the vmbus core. You aren't doing that at all, and
you probably need to make sure that this isn't needed. That is
concentrating on the vmbus driver.
I audited the block and the net drivers. As part of their exit routine,
they invoke vmbus_child_driver_unregister() after properly cleaning
up all the devices they are managing. Do you still see an issue with
regards to module reference counting.
I will look again, the next time I review the vmbus code.
Thank you.
I will also address your comment on static initialization hv_driver
instances
as part of other driver cleanup.
No, please do this now as it will show how to properly interact with the
vmbus core code in the correct manner. Hopefully that will be correct,
but I have a feeling that it will show you some places in the API that
need to be changed...
As opposed to run-time initialization of fields such as probe, etc; I have
initialized them statically. For instance, in the blkvsc driver:
/* The one and only one */
static struct storvsc_driver blkvsc_drv = {
.base.probe = blkvsc_probe,
.base.remove = blkvsc_remove,
.base.shutdown = blkvsc_shutdown,
};
Is this what you had in mind.
Close. The format is correct.
But what's with that .base. crud? That shows that something is wrong
as no USB or PCI or any other bus driver has to mess with a .base
subpointer in a driver structure.
See, I told you that when you converted to use this format the problems
would pop out at you :)
Please fix that.
I will. I am incrementally cleaning up the drivers, I will cleanup the .base
crud as part of additional cleanup that I will do.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, May 03, 2011 4:47 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: various vmbus review comments I just took a quick look at the vmbus code, and have the following comments: - why is count_hv_channel() even a function? I will fix this. - your .h files need to be consolidated and renamed. You will need a single hyperv.h file for include/linux/ that will contain some of what the vmbus*.h files have in it, but not all. Please merge things together to have: - include/linux/hyperv.h What is needed to build the drivers that attach to the bus - drivers/staging/hv/hyperv.h The local .h file will have the vmbus*.h remaining stuff that is only needed by the hv_vmbus.ko module to be build. I have currently consolidated all the header files as follows: 1) hyperv.h - this will have all the vmbus related definitions needed to build drivers that attach to the bus (as you have suggested). 2) hyperv_storage.h - this has all the definitions needed to build storage drivers for Hyper-V. Storage drivers will include hyperv.h and hyperv_storage.h. 3) hyperv_net.h - this has all the definitions needed to build the network driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h. 4) hyperv_utils.h - this has all the definitions needed to build the util driver. The util driver would include hyperv.h and hyperv_utils.h. All these four header files could potentially be under include/linux. I have also created a private header file that has the definitions to build the vmbus driver - hyperv_vmbus.h. Let me know if this is what you had in mind. - the instances of hv_driver structures need to be static and not programatically defined, like all other USB and PCI drivers are handled. - module reference counting. Are you sure you got it all right for the individual modules that attach to the bus? I don't see any reference counting happening, is that correct? For this round, I want to concentrate on just the vmbus driver. So, module reference counting is I don't think an issue for the vmbus driver given that the driver is not unlodable. Once I am done with the vmbus driver I will address the module reference counting issues for other drivers. I will also address your comment on static initialization hv_driver instances as part of other driver cleanup. - fix the sparse warnings. Will do. Just out of curiosity I ran sparse and was relieved to see only a handful warnings! - fix the use of volatile in the ring buffer code. It should not be needed and if you are relying on it, then the code is wrong. - fix the namespace on the ringbuffer code to show that it really is only for the hyperv bus code internally. I will fix these. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: various vmbus review comments
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, May 04, 2011 12:32 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: various vmbus review comments On Wed, May 04, 2011 at 04:20:11PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, May 03, 2011 4:47 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: various vmbus review comments I just took a quick look at the vmbus code, and have the following I have currently consolidated all the header files as follows: 1) hyperv.h - this will have all the vmbus related definitions needed to build drivers that attach to the bus (as you have suggested). Great. 2) hyperv_storage.h - this has all the definitions needed to build storage drivers for Hyper-V. Storage drivers will include hyperv.h and hyperv_storage.h. 3) hyperv_net.h - this has all the definitions needed to build the network driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h. 4) hyperv_utils.h - this has all the definitions needed to build the util driver. The util driver would include hyperv.h and hyperv_utils.h. No for all 3 of these. Why would a simple driver need a .h file at all? Just include the structures within the .c file, making it nice and self-contained. There's no need to clutter things up, and individual driver .h files should _never_ be in include/linux. Agreed - we don't need to clutter up include/linux directory with individual driver.h files. However, including all the definitions in the .c file would also be ugly. For instance there are multiple *.c files for the storage drivers and we don't want to replicate the contents of hyperv_storage.h in each of these .c files. The same is true for the netvsc driver - there are multiple .c files where we would need to replicate the contents of hyperv_net.h. How about I keep these driver specific header files; but these could be local to the directory where these drivers land. - the instances of hv_driver structures need to be static and not programatically defined, like all other USB and PCI drivers are handled. - module reference counting. Are you sure you got it all right for the individual modules that attach to the bus? I don't see any reference counting happening, is that correct? For this round, I want to concentrate on just the vmbus driver. So, module reference counting is I don't think an issue for the vmbus driver given that the driver is not unlodable. Once I am done with the vmbus driver I will address the module reference counting issues for other drivers. No, I am referring to the module reference counting of the bus drivers that register with the vmbus core. You aren't doing that at all, and you probably need to make sure that this isn't needed. That is concentrating on the vmbus driver. Ok; I will look into this. I will also address your comment on static initialization hv_driver instances as part of other driver cleanup. No, please do this now as it will show how to properly interact with the vmbus core code in the correct manner. Hopefully that will be correct, but I have a feeling that it will show you some places in the API that need to be changed... Will do. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
